Repository navigation
Fixing typos - #617
Fixing typos#617
Conversation
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 2m 33s —— View job Review of PR #617: Fixing typos
Most of the PR is good: the renames are applied consistently across the Java sources, and the 🔴 Blocking
Neither file is in the diff, so I couldn't attach a one-click suggestion. Both need a find/replace. 🟠 Should fix
🟡 Consider
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of the typo-fix PR. Most changes look good: the renames are applied consistently across the Java sources, and the @since normalisation and @SuppressWarnings("SpellCheckingInspection") additions are fine.
Blocking
_CSharpMacroLexer.flexandCSharpPreprocessorHighlightLexer.flexstill referenceCSharpPreprocesorTokens. These lexers are generated at build time, socsharp-psi-implwon't compile.tests/src/test/resources/parsing/Issue267.txtstill expectsCSharpAnonymFieldOrPropertySetImpl, so the parsing test will fail.
Should fix
3. getGlobalUsings → getGlobalUsages is a semantic regression: these are C# global using directives, not usages.
Consider
4. Several public API renames (the visitor method, OverrideUtil, a psi-api interface) can break downstream plugins.
5. A few comment and grammar nits have suggestions inline. There's also a minor inconsistency in CSharpParenthesesInsertHandler: lParenthOffset was renamed but rparenthOffset wasn't.
| * @since 2014-01-23 | ||
| */ | ||
| public interface CSharpPreprocesorTokens extends StandardTokenTypes { | ||
| public interface CSharpPreprocessorTokens extends StandardTokenTypes { |
There was a problem hiding this comment.
🔴 Build break: the lexers are generated at build time from .flex sources (generate-lexers goal in csharp-psi-impl/pom.xml), and both still reference the old name:
lexer/_CSharpMacroLexer.flex:import ...CSharpPreprocesorTokens;+ 29 usageslexer/CSharpPreprocessorHighlightLexer.flex:import ...CSharpPreprocesorTokens;+ 21 usages
Those files aren't in the diff, so no suggestion block is possible here. Replace CSharpPreprocesorTokens → CSharpPreprocessorTokens in both .flex files. (While there: %class CSharpPreprocessorHightlightLexer is also a typo, but renaming it touches CSharpPreprocessorSyntaxHighlighter and CSharpHighlightLexer2 too.)
| * @since 2015-04-07 | ||
| */ | ||
| public class CSharpAnonymFieldOrPropertySetImpl extends CSharpElementImpl implements CSharpAnonymFieldOrPropertySet | ||
| public class CSharpAnonymousFieldOrPropertySetImpl extends CSharpElementImpl implements CSharpAnonymousFieldOrPropertySet |
There was a problem hiding this comment.
🔴 Test failure: PSI tree dumps include the impl class name, and tests/src/test/resources/parsing/Issue267.txt (lines 95 and 103) still expects
CSharpAnonymFieldOrPropertySetImpl(ANONYM_FIELD_OR_PROPERTY_SET)
so the Issue267 parsing test will fail. Update it to CSharpAnonymousFieldOrPropertySetImpl(ANONYM_FIELD_OR_PROPERTY_SET).
(Keeping the ANONYM_FIELD_OR_PROPERTY_SET element-type debug name avoids more test churn, which is fine.)
|
|
||
| @RequiredReadAction | ||
| default Collection<String> getGlobalUsings() | ||
| default Collection<String> getGlobalUsages() |
There was a problem hiding this comment.
🟠 This changes the meaning. Usings was not a typo: it refers to C# global using directives (global using System.Linq;), and the method returns their namespace qNames. Usages means "places where something is used", which is misleading. The private helper CSharpFileImpl.getGlobalUsingsImpl() also still has the old name. And this is a public API method on CSharpFile (psi-api), so the rename can break other plugins. I'd suggest reverting it, along with CSharpFileImpl:127 and CSharpResolveUtil:208:
| default Collection<String> getGlobalUsages() | |
| default Collection<String> getGlobalUsings() |
Alternatively, use something unambiguous like getGlobalUsingNamespaces().
| @RequiredReadAction | ||
| @Override | ||
| public Collection<String> getGlobalUsings() | ||
| public Collection<String> getGlobalUsages() |
There was a problem hiding this comment.
Revert this together with CSharpFile#getGlobalUsings (see the comment there):
| public Collection<String> getGlobalUsages() | |
| public Collection<String> getGlobalUsings() |
| if(containingFile instanceof CSharpFile file) | ||
| { | ||
| for(String qName : file.getGlobalUsings()) | ||
| for(String qName : file.getGlobalUsages()) |
There was a problem hiding this comment.
| for(String qName : file.getGlobalUsages()) | |
| for(String qName : file.getGlobalUsings()) |
| if(type != null) | ||
| { | ||
| // disable native type completion, due they dont extends System.Exception | ||
| // disable native type completion, due to they don't extend System.Exception |
There was a problem hiding this comment.
Grammar nit ("due to" needs a noun):
| // disable native type completion, due to they don't extend System.Exception | |
| // disable native type completion, because they don't extend System.Exception |
| } | ||
| } | ||
| // dont return array if size of elements if not equal tokens | ||
| // Don't return array if size of elements if not equal to tokens |
There was a problem hiding this comment.
| // Don't return array if size of elements if not equal to tokens | |
| // Don't return array if size of elements is not equal to tokens |
| if(!(qualifiedElement instanceof DotNetModifierListOwner)) | ||
| { | ||
| // dont interest if we dont have parent - how it can be? | ||
| // Doesn't interest if we don't have parent - how can it be? |
There was a problem hiding this comment.
| // Doesn't interest if we don't have parent - how can it be? | |
| // Not interested if there is no parent - how can that happen? |
| // If no modifiers but we failed to parse type - need go advance, due to ill infinity loop inside parseParameterList, | ||
| // but don't eat closing brace |
There was a problem hiding this comment.
| // If no modifiers but we failed to parse type - need go advance, due to ill infinity loop inside parseParameterList, | |
| // but don't eat closing brace | |
| // If there are no modifiers but we failed to parse type - we need to advance, otherwise there will be an infinite loop inside parseParameterList, | |
| // but don't eat closing brace |
| final int parameterSize) | ||
| { | ||
| //TODO [VISTALL] some hack until we dont make override more powerfull | ||
| //TODO [VISTALL] some hack until we don't make override more powerful |
There was a problem hiding this comment.
The double negative flips the meaning:
| //TODO [VISTALL] some hack until we don't make override more powerful | |
| //TODO [VISTALL] some hack until we make override more powerful |
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 1m 50s —— View job Re-review of PR #617: Fixing typos
I didn't run the Maven build or the tests. I checked the fixes by searching the code with ✅ Fixed since the last review
🟡 New inline comments
Still open from the last review (optional)I didn't re-post these because the earlier threads are still open:
None of these block the merge. If the build and tests pass, the PR looks good to merge. |
| final int parameterSize) | ||
| { | ||
| //TODO [VISTALL] some hack until we dont make override more powerfull | ||
| //TODO [VISTALL] some hack until we don make override more powerful |
There was a problem hiding this comment.
Typo introduced by this PR (don instead of do not). The original "until we dont make" was also a double negative. This is what is meant:
| //TODO [VISTALL] some hack until we don make override more powerful | |
| //TODO [VISTALL] some hack until we make override more powerful |
| if(completion) | ||
| { | ||
| // append generic when completion due at ANY_MEMBER it dont resolved | ||
| // append generic on completion if at ANY_MEMBER it isn't resolved |
There was a problem hiding this comment.
The original due at ANY_MEMBER it dont resolved meant because generic parameters are not resolved at ANY_MEMBER. With if, it reads like a runtime condition, which it is not:
| // append generic on completion if at ANY_MEMBER it isn't resolved | |
| // Append generic parameters on completion, because they are not resolved at ANY_MEMBER |
| } | ||
|
|
||
| private static class ParamatizeValue | ||
| private static class ParametrizedValue |
There was a problem hiding this comment.
Nit: ParamatizeValue → ParametrizedValue fixes the typo, but this class caches resolve results keyed by qualifier element, and neither name says that. Something like QualifierResolveCache would be clearer. It is private, so the rename is safe. If you keep this name, "Parameterized" is the more common spelling.
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 1m 2s —— View job Re-review of PR #617: Fixing typos (round 3)
I didn't run the Maven build or the tests. I checked every changed line in the diff by reading it. ✅ Fixed since the last review
New problemsNone. The other changes all look right:
Still open (optional, not re-posted)The earlier threads for these are still open:
Nothing blocks the merge. If CI builds and the tests pass, this looks good to merge. |
2 participants
No description provided.