Repository navigation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] The change looks right and matches the existing convention for
EVM/NFT/RPC/etc. One thing worth confirming from the Vale run you did: the description listsevm/tokens.mdx:14(#### ERC721 NFTs) among the cleared warnings, but that heading uses the unhyphenatedERC721. If Vale matches exceptions as whole tokens (\bERC\b),ERC721would not match — there is no word boundary betweenCand7— and that warning should not have cleared. It clearing implies exception matching is substring-based, which would also meanERCexempts any word containing an uppercaseERC(harmless in practice: no realistic heading word other than an ERC standard contains uppercaseERC).Not blocking either way — this rule is
level: warningandprose-style.ymlruns withfail_on_error: false, so nothing gates on it. Just worth a sentence in the PR body clarifying which matching semantics you observed, since it determines whether future unhyphenatedERCnnnheadings are covered by this entry or need their own.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and your inference is right: matching is substring-based, not
\bERC\b. I tested it by running Vale over a probe file against two copies of the styles directory, identical except for this one line.Flagged without the entry, clean with it:
## ERC20 tokens## ERC721 nfts## ERC721 NFTsStill flagged with the entry:
## ERC20 Tokens,## ERC-20 Tokens,## ERC Tokens,## EVM Tokens,## Foo Tokens.So unhyphenated
ERCnnnis covered by this single entry, and futureERCnnnheadings will not need their own. I have added a paragraph to the PR description recording those semantics.Your follow-up point deserves a correction on my side, though. My original description claimed the entry "does not hide genuine title-case violations," and that was too strong. It is not perfectly surgical:
Batching ERC-20 Reads with viemand the threeERC-nnn Interactiontitles each have a second capitalized word, and they stop being annotated too. Worth noting that imprecision is not specific to this entry, since## Foo Interactionnever warned either, so the rule was already lenient about a trailing capitalized word in short headings.That leaves a choice I am happy to defer to you. Either keep it as is, accepting that four imperfect pre-existing headings go unannotated, or I fix those four headings' capitalization in this PR so the rule stays strict. Say which you prefer.