-
Notifications
You must be signed in to change notification settings - Fork 66
add spell checker in CI/CD and Makefile #1582
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
Conversation
|
Not sure we should keep definitions/contributed_definitions/NXlab_electro_chemo_mechanical_preparation.nxdl.xml Lines 77 to 78 in 023c5f0
Can we improve this wording? Perhaps
|
|
The word |
|
Custom dictionary: |
|
Custom dictionary: |
|
Thanks for the suggestion @prjemian! The custom-dictionary as it is was copied over from our fork of the repository where we have some additional classes that are not yet in this repo. I suggest we wait with this PR here until at least #1581 and #1423 (where some of the contributed definitions are also removed) are done, then I will update the custom word list again and remove those words that are not actually needed. |
|
@phyy-nx @prjemian this should be ready now. I went to the custom dictionary again and checked that every word has meaning and is not a typo. I also ran two bash script that explicitly check 1) that each file in the custom dict is actuall used somewhere, and 2) that none of the words from the custom dict are already used in the imported dictionaries from cspell. |
|
Need all comments resolved before merging :) |
|
In fact, a bit of shell scripting gives this list of unused "words":
|
…ith some terms on Peters side suggested that they are unused while cspell identifies them as used and warns about them no in the custom dictionary. Therefore, made a copy of the previous list, removed grouping, sorted lexi ascending and removed duplicates. Now will run the CI and a script that fishes all unknown words and then compare, strongly suggest to remove the grouping but instead sort all lexicographically ascending
…nary more changes observed than revealed by the review process, compared changes between that list and the previously used list when that was also sorted, like one-on-one comparison
|
@PeterC-DLS thank you for your careful feedback, observed differences also in between your scripting and running with a virgin custom-dictionary --- that change in strategy solved the issue, compared the resulting custom dictionary with the previous one that you reviewed prior fa46697, overall number of custom fragments/words remains but changes also in other places, eventually changes on the cspell dictionary side and processing strategy while this PR evolved also had an effect. I checked that the side-by-side comparison of the prior fa46697 dict and the present 1afe did not add terms that are unreasonable from a human perspective. |
|
Mind this PR's source is on FAIRmat-NFDI directed towards the NIAC repo, we keep the FAIRmat-NFDI/main synced up with nexusformat/definitions/main while work was pursued on the spellcheck branch the niac main changed 1afe2d4 synced this up |
|
Can we merge in the typos part of this and delay the CI stuff until the next release? In other words, remove all the cspell stuff from this PR and punt that down the road, but get the useful changes in now? |
…f cspell down the road but take the corrections this experiment brought about
… undesired external dependency
@phyy-nx done |
|
Great. Just needs an approval. @PeterC-DLS ? |
PeterC-DLS
left a comment
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.
All looks fine to me
We removed the make target, so the review is no longer applicable
This adds a spellchecker (
cspell) and introduces a custom dictionary for it. In the course of implementing this, a couple of typos were fixed, mostly in the contributed definitions.