Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Trivial] Add Spell Checker rules to '.editorconfig' #10311

Merged
merged 9 commits into from Apr 4, 2023

Conversation

yahiheb
Copy link
Collaborator

@yahiheb yahiheb commented Mar 19, 2023

@kiminuo
Copy link
Collaborator

kiminuo commented Mar 20, 2023

I like this PR. There is also https://learn.microsoft.com/en-us/visualstudio/ide/text-spell-checker?view=vs-2022 (maybe worth adding to your OP).

Personally, I would try to add spelling_exclusion_path and add a coinjoin entry to that file to ignore it because it's AFAIK unknown word (at this point).

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK. The severity of the spelling issue is not an error, but only info. We should keep errors for code problems. Add spelling_exclusion_path so we together can build a project-wide dictionary.

@yahiheb
Copy link
Collaborator Author

yahiheb commented Mar 26, 2023

Personally, I would try to add spelling_exclusion_path and add a coinjoin entry to that file to ignore it because it's AFAIK unknown word (at this point).

Done.

cACK. The severity of the spelling issue is not an error, but only info. We should keep errors for code problems. Add spelling_exclusion_path so we together can build a project-wide dictionary.

Done.

turbolay
turbolay previously approved these changes Mar 26, 2023
@kiminuo
Copy link
Collaborator

kiminuo commented Mar 26, 2023

I wonder why exclusion.dic is reported by Github as a binary file. Hm.

@yahiheb yahiheb requested a review from turbolay March 27, 2023 01:40
kiminuo
kiminuo previously approved these changes Mar 27, 2023
Copy link
Collaborator

@kiminuo kiminuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and it seems to work

I would add segwit & Segwit & Prevout to the dictionary too.

.editorconfig Outdated
spelling_languages = en-us
spelling_checkable_types = strings,identifiers,comments
spelling_error_severity = information
spelling_exclusion_path = .\exclusion.dic
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it would be .\exclusion.txt
Can you check if the spellchecker can handle it?

It would fix the following:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but it didn't work.

Copy link
Collaborator

@kiminuo kiminuo Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wild guess: https://git-scm.com/docs/gitattributes#_marking_files_as_binary .. use .gitattributes to mark .dic as non-binary file. But maybe not worth the effort, I'm not sure. I'm not sure it will fix the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth the effort otherwise we won't see the changes in the dictionary file. @yahiheb pls check this to solve the problem https://stackoverflow.com/questions/6855712/why-does-git-treat-this-text-file-as-a-binary-file

Copy link
Collaborator

@kiminuo kiminuo Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the issue here is that exclusion.dic has UTF-16 BOM encoding. Here https://github.com/zkSNACKs/WalletWasabi/pull/10410/files#diff-85ffb40cdc41bba97c28132b5f729d5fdfeb4f61b919eb8fcf35c0747a6d34f9 you can see that if the file is in UTF-8, it's OK. But the question is whether the spell checker should then work. They say it should but for me, it works with Visual Studio preview but not Visual Studio (stable).

So there are options:

  • Wait for the next VS release.
  • Merge as is and change after the next VS is released.
  • Try UTF-16 without BOM.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so either wait or merge as-is and change after MSVS 17.6 is released (might take weeks, not more than 2 months I guess).

I would just merge it, the feature is useful.

@molnard
Copy link
Collaborator

molnard commented Mar 30, 2023

What's up with this @yahiheb ?

@yahiheb
Copy link
Collaborator Author

yahiheb commented Mar 30, 2023

I would add segwit & Segwit & Prevout to the dictionary too.

Done, but there are a lot of words to add in another PR.

What's up with this @yahiheb ?

It is ready.

@molnard
Copy link
Collaborator

molnard commented Mar 31, 2023

It is ready.

It is not good until we cannot see the words on GitHub. The dic will be enormous.

@kiminuo
Copy link
Collaborator

kiminuo commented Apr 1, 2023

The dic will be enormous.

Hm, I don't think it will be that bad. I would expect that like 50 entries will cover 80% of all spell-checker warnings. Now let's hope this will age well. Haha.

.editorconfig Outdated
@@ -289,3 +289,9 @@ dotnet_diagnostic.CA1827.severity = warning

# CA1822: This complains to make functions static. Static is evil. Don't let it complain.
dotnet_diagnostic.CA1822.severity = none

# Spell Checker rules
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would put the spell checker rules after L27. Nobody will scroll down here because it's reasonable to think that only diagnostics follow.

Feel free to ignore though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@yahiheb
Copy link
Collaborator Author

yahiheb commented Apr 2, 2023

As @kiminuo has mentioned here #10311 (comment), the feature is useful and I would merge it now, and change after MSVS 17.6 is released.

@molnard
Copy link
Collaborator

molnard commented Apr 3, 2023

the feature is useful and I would merge it now

With one condition: whenever there is a change in exclusion.dic you have to approve it.

@yahiheb
Copy link
Collaborator Author

yahiheb commented Apr 3, 2023

the feature is useful and I would merge it now

With one condition: whenever there is a change in exclusion.dic you have to approve it.

ACK

@molnard molnard merged commit cfa94cd into zkSNACKs:master Apr 4, 2023
7 checks passed
@yahiheb yahiheb deleted the spell branch April 4, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants