doc: spell out condition restrictions #55187
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
jkrems
force-pushed
the
jk-condition-restrictions
branch
from
d8704aa
to
e2fcec8
Compare
jkrems
added a commit
to jkrems/node
that referenced
this pull request
Sep 30, 2024
jkrems
force-pushed
the
jk-condition-restrictions
branch
from
e2fcec8
to
f71b086
Compare
guybedford
reviewed
Sep 30, 2024
doc/api/packages.md
Outdated
Comment on lines
692
to
693
| Conditions can be almost any string, including multi-byte characters and | ||
| whitespace. There's only a few restrictions: |
There was a problem hiding this comment.
I would prefer that we do not say this, and instead say something less specific, eg that special characters are permitted in condition names. We should probably ban spaces IMO.
There was a problem hiding this comment.
We could start this with a stronger recommendation (e.g. keep it to /^[a-z0-9:=-]+$/) and only afterwards go into the technical side of what's enforced?
I'm +1 on banning whitespace and also banning quotes and non-ASCII characters. I don't see a clear use case for those but could see many confusing bugs. Conditions usually fail silently, so debugging a "looked like character A but was actually character B" problem seems super annoying.
There was a problem hiding this comment.
Added a more opinionated opening paragraph before delving into the technical restrictions.
jkrems
added a commit
to jkrems/node
that referenced
this pull request
Sep 30, 2024
jkrems
force-pushed
the
jk-condition-restrictions
branch
from
f71b086
to
5c50184
Compare
jkrems
added a commit
to jkrems/node
that referenced
this pull request
Sep 30, 2024
jkrems
force-pushed
the
jk-condition-restrictions
branch
from
5c50184
to
b223c49
Compare
|
@guybedford Let me know if you have objections to landing, otherwise I'm planning to land later today or tomorrow. :) |
guybedford
reviewed
Oct 15, 2024
jkrems
force-pushed
the
jk-condition-restrictions
branch
from
b223c49
to
9e167f3
Compare
jkrems
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/55187 ✔ Done loading data for nodejs/node/pull/55187 ----------------------------------- PR info ------------------------------------ Title doc: spell out condition restrictions (#55187) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch jkrems:jk-condition-restrictions -> nodejs:main Labels doc Commits 1 - doc: spell out condition restrictions Committers 1 - Jan Martin <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55187 Reviewed-By: Matteo Collina <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55187 Reviewed-By: Matteo Collina <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - doc: spell out condition restrictions ℹ This PR was created on Mon, 30 Sep 2024 18:09:00 GMT ✔ Approvals: 1 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55187#pullrequestreview-2369916989 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11369002902 |
jkrems
added a commit
that referenced
this pull request
Oct 16, 2024
PR-URL: #55187 Reviewed-By: Matteo Collina <[email protected]>
|
Landed in 73414f3 |
aduh95
pushed a commit
that referenced
this pull request
Oct 19, 2024
PR-URL: #55187 Reviewed-By: Matteo Collina <[email protected]>
Merged
louwers
pushed a commit
to louwers/node
that referenced
this pull request
Nov 2, 2024
PR-URL: nodejs#55187 Reviewed-By: Matteo Collina <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I never really thought about what kinds of conditions are allowed and which aren't - and it looks like the answer is mostly "anything goes". This documents the status-quo from a practical perspective from what I can gather.
Some of these restrictions (e.g. "no comma") aren't actively enforced by nodejs but maybe should be in the future.