docs/docs-template
Template
20
1
Fork 3
Code Issues 2 Pull requests Projects Activity Actions

feat: Configurable docsbuilder port #20

Merged
pbokoc merged 3 commits from docs_listen_port into main 2026-05-27 22:22:07 +00:00
Conversation 14 Commits 3 Files changed 1 +47 -41
mwinters commented 2026-05-20 20:11:37 +00:00
Member
Copy link

Since this is set as an environment variable, you can use tools such as direnv or mise to set per-directory values when you cd into them, ensuring that none of your local docs repos have port conflicts.

Since this is set as an environment variable, you can use tools such as [direnv](https://direnv.net/) or [mise](https://mise.jdx.dev/) to set per-directory values when you `cd` into them, ensuring that none of your local docs repos have port conflicts.
mwinters commented 2026-05-20 20:13:59 +00:00
Author
Member
Copy link

I wasn't sure whether you'd prefer that I bump the script version in the PR or later on main

I wasn't sure whether you'd prefer that I bump the script version in the PR or later on main
hricky commented 2026-05-21 08:13:43 +00:00
Owner
Copy link

@mwinters wrote in #20 (comment):

I wasn't sure whether you'd prefer that I bump the script version in the PR or later on main

I'm not entirely sure what you mean by that. I also notice, IIUC, that you created a branch for this PR directly in this repo without forking it first. Is that the recommended approach for contributing to this repo?

@mwinters wrote in https://forge.fedoraproject.org/docs/docs-template/pulls/20#issuecomment-713865: > I wasn't sure whether you'd prefer that I bump the script version in the PR or later on main I'm not entirely sure what you mean by that. I also notice, IIUC, that you created a branch for this PR directly in this repo without forking it first. Is that the recommended approach for contributing to this repo?
hricky commented 2026-05-21 09:16:54 +00:00
Owner
Copy link

Also, the pattern : "${DOCS_LISTEN_PORT:=8080}" is an unusual way to set a default. It's clearer to use DOCS_LISTEN_PORT=${DOCS_LISTEN_PORT:=8080} or a simple conditional check. Additionally, this breaks the script's naming convention. All other variables are lowercase (script_name, srcdir, buildir, etc.). Consider using docs_listen_port instead.

Also, the pattern `: "${DOCS_LISTEN_PORT:=8080}"` is an unusual way to set a default. It's clearer to use `DOCS_LISTEN_PORT=${DOCS_LISTEN_PORT:=8080}` or a simple conditional check. Additionally, this breaks the script's naming convention. All other variables are lowercase (`script_name`, `srcdir`, `buildir`, etc.). Consider using `docs_listen_port` instead.
mwinters commented 2026-05-21 14:05:14 +00:00
Author
Member
Copy link

I'm not entirely sure what you mean by that [script version].

Line 21 in fa8418b
version="1.2.0"

Is that the recommended approach for contributing to this repo?

This is the general FOSS norm when one has maintainer privileges. Is that not the norm here? What value would be added by the extra process steps?

an unusual way to set a default

I suppose "unusual" depends on what scripts you've been looking at lately 🙂. I can change this to whatever is "usual" here. (Commit incoming.)

this breaks the script's naming convention. All other variables are lowercase (script_name, srcdir, buildir, etc.).

This variable is intended to come from the environment, where POSIX says variables should be all-caps:

Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2017 consist solely of uppercase letters, digits, and the ( '_' )

Additionally, the Google Bash Style Guide, which is arguably one of the primary authoritative sources on style besides POSIX, says:

Constants and anything exported to the environment should be capitalized, separated with underscores, and declared at the top of the file.

This matches my experience from bash around many other projects. So actually, this script does not conform to the recommended styles from several authoritative sources. Would you like me to update it?

> I'm not entirely sure what you mean by that \[script version]. https://forge.fedoraproject.org/docs/docs-template/src/commit/fa8418b2be3bf9524189c0ec59445edbfb9f8936/docsbuilder.sh#L21 --- > Is that the recommended approach for contributing to this repo? This is the general FOSS norm when one has maintainer privileges. Is that not the norm here? What value would be added by the extra process steps? --- > an unusual way to set a default I suppose "unusual" depends on what scripts you've been looking at lately 🙂. I can change this to whatever is "usual" here. (Commit incoming.) --- > this breaks the script's naming convention. All other variables are lowercase (script_name, srcdir, buildir, etc.). This variable is intended to come from the environment, where [POSIX says variables should be all-caps](https://pubs.opengroup.org/onlinepubs/9699919799/): > Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2017 consist solely of uppercase letters, digits, and the <underscore> ( '_' ) Additionally, the [Google Bash Style Guide](https://google.github.io/styleguide/shellguide.html#constants-environment-variables-and-readonly-variables), which is arguably one of the primary authoritative sources on style besides POSIX, says: > Constants and anything exported to the environment should be capitalized, separated with underscores, and declared at the top of the file. This matches my experience from bash around many other projects. So actually, this script does not conform to the recommended styles from several authoritative sources. Would you like me to update it?
hricky commented 2026-05-21 15:03:26 +00:00
Owner
Copy link

@mwinters wrote in #20 (comment):

This is the general FOSS norm when one has maintainer privileges. Is that not the norm here? What value would be added by the extra process steps?

In the projects I participate in, everyone opens PRs from their forks, even those with write permissions do so. The only exception is when we're testing something in a non-production environment, like the Fedora Forge staging instance for example, which isn't the case here. Since I'm a new member of the team, I don't know what the norm is, so I'm asking.

@mwinters wrote in https://forge.fedoraproject.org/docs/docs-template/pulls/20#issuecomment-742701: > This is the general FOSS norm when one has maintainer privileges. Is that not the norm here? What value would be added by the extra process steps? In the projects I participate in, everyone opens PRs from their forks, even those with write permissions do so. The only exception is when we're testing something in a non-production environment, like the Fedora Forge staging instance for example, which isn't the case here. Since I'm a new member of the team, I don't know what the norm is, so I'm asking.
mwinters commented 2026-05-21 15:33:16 +00:00
Author
Member
Copy link

@pbokoc Care to weigh in with a preference? Should members of the docs team still be required to use personal forks for PRs, or are branch PRs ok?

My $0.02: if I'm already a member of the team and presumably committing somewhat frequently, then I don't see what value we're adding by requiring the extra friction of a fork. We just need to delete branches when merging PRs. And if we're really worried, we could prefix PR branches like mwinters/foo or feature/bar.

@pbokoc Care to weigh in with a preference? Should members of the docs team still be required to use personal forks for PRs, or are branch PRs ok? My $0.02: if I'm already a member of the team and presumably committing somewhat frequently, then I don't see what value we're adding by requiring the extra friction of a fork. We just need to delete branches when merging PRs. And if we're really worried, we could prefix PR branches like `mwinters/foo` or `feature/bar`.
hricky commented 2026-05-21 18:00:21 +00:00
Owner
Copy link

AIUI, the POSIX page applies to environment variables provided by the system or used by POSIX utilities, not user-defined script variables.

As you mentioned, this variable is intended to come from the environment. The Google Bash Style Guide states that anything exported to the environment should be capitalized. AIUI, this refers to variables you explicitly export with export or declare -x to pass to child processes.

DOCS_LISTEN_PORT is not exported to the environment. It's a user-defined script variable that accepts environment input via the ${VAR:-default} syntax.

AIUI, the POSIX page applies to environment variables provided by the system or used by POSIX utilities, not user-defined script variables. As you mentioned, this variable is intended to come from the environment. The Google Bash Style Guide states that anything exported to the environment should be capitalized. AIUI, this refers to variables you explicitly export with `export` or `declare -x` to pass to child processes. `DOCS_LISTEN_PORT` is not exported to the environment. It's a user-defined script variable that accepts environment input via the `${VAR:-default}` syntax.
mwinters commented 2026-05-21 18:53:47 +00:00
Author
Member
Copy link

It is also a constant, i.e. the value is never modified in the script after definition. Which, in every bash script I've ever read, is supposed to be in all-caps. And so we arrive back at the original variable name.

It is also a constant, i.e. the value is never modified in the script after definition. Which, in every bash script I've ever read, is supposed to be in all-caps. And so we arrive back at the original variable name.
hricky commented 2026-05-21 21:22:21 +00:00
Owner
Copy link

According to the Google Shell Style Guide, variables should be uppercase only if they are declared with readonly or export. This variable is neither. Simply not modifying a variable doesn't make it a constant.

If you declare it readonly DOCS_LISTEN_PORT="${DOCS_LISTEN_PORT:=8080}", it technically works but is semantically confusing.

According to the Google Shell Style Guide, variables should be uppercase only if they are declared with `readonly` or `export`. This variable is neither. Simply not modifying a variable doesn't make it a constant. If you declare it `readonly DOCS_LISTEN_PORT="${DOCS_LISTEN_PORT:=8080}"`, it technically works but is semantically confusing.
- All constants are `readonly UPPERCASE`
- Consistent indentation
- Variables are quoted wherever appropriate
- `local` is applied to local variables
- bash `[[ ]]` is used where appropriate

Co-authored-by: Claude Code <noreply@anthropic.com>
mwinters commented 2026-05-22 20:01:18 +00:00
Author
Member
Copy link

Ok, the single-variable feature is added AND I've addressed several code consistency / best-practices issues across the entire script:

  • All constants are readonly UPPERCASE
  • Consistent indentation
  • Variables are quoted wherever appropriate
  • local is applied to local variables
  • bash [[ ]] is used where appropriate
Ok, the single-variable feature is added AND I've addressed several code consistency / best-practices issues across the entire script: - All constants are `readonly UPPERCASE` - Consistent indentation - Variables are quoted wherever appropriate - `local` is applied to local variables - bash `[[ ]]` is used where appropriate
pbokoc commented 2026-05-26 10:12:03 +00:00
Owner
Copy link

Hmmm. So re: opening PRs from forks vs directly - I don't have a preference, I have to admit I never even thought of that. I've always used a fork, and I sometimes just push directly if it's something small.

I'll open a separate issue in tickets so we can maybe talk about it in the next team meeting. (Edit: docs/tickets#46)

As for the rest of the PR - can we merge now or is there something still missing?

Hmmm. So re: opening PRs from forks vs directly - I don't have a preference, I have to admit I never even thought of that. I've always used a fork, and I sometimes just push directly if it's something small. I'll open a separate issue in `tickets` so we can maybe talk about it in the next team meeting. (Edit: https://forge.fedoraproject.org/docs/tickets/issues/46) As for the rest of the PR - can we merge now or is there something still missing?
mwinters commented 2026-05-27 21:00:41 +00:00
Author
Member
Copy link

@pbokoc wrote in #20 (comment):

is there something still missing?

Not to my knowledge 🙂. Just a version bump after merge.

@pbokoc wrote in https://forge.fedoraproject.org/docs/docs-template/pulls/20#issuecomment-760071: > is there something still missing? Not to my knowledge 🙂. Just a version bump after merge.
pbokoc approved these changes 2026-05-27 22:21:36 +00:00
pbokoc commented 2026-05-27 22:21:56 +00:00
Owner
Copy link

Alright, cool, thank you everyone!

Alright, cool, thank you everyone!
pbokoc merged commit f56accbdd4 into main 2026-05-27 22:22:07 +00:00
pbokoc deleted branch docs_listen_port 2026-05-27 22:22:08 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
pbokoc
Labels
Clear labels
  
effort
high
 Will take a long time

  
effort
low
 Easy fix

  
effort
medium
 Will take some time

  
good first issue
Take this if you're new!

  
help wanted
Needs help or feedback from the wider Fedora community

  
meeting topic
Need to discuss this at a Docs team meeting

  
needs changes
Needs changes or fixes before closing

  
needs reporter feedback
Additional feedback or information from the reporter is needed to advance

  
needs review
Someone needs to look at this

  
priority
high
 Fix this NOW NOW NOW!

  
priority
low
 Low priority tasks

  
priority
medium
 Medium priority tasks

  
priority
on hold
 Work has been temporarily halted, usually because another task needs to be completed first.

  
type/content
Changes to the contents of the site - the actual docs

  
type/misc
Changes to non-content parts of the site. CSS, Actions, scripts, etc.

No labels
effort
high
effort
low
effort
medium
good first issue
help wanted
meeting topic
needs changes
needs reporter feedback
needs review
priority
high
priority
low
priority
medium
priority
on hold
type/content
type/misc
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
docs/docs-template!20
No description provided.