Feat: Added Spell-Checker and fixed spellings#208
Conversation
For Code Spell Checker extension. Fixed some clippy warnings.
GunterSchmidt
force-pushed
the
add_cspell_project_dictionaries
branch
from
af533ab to
2646457
Compare
|
@sylvestre Could you take a look at this? I then can continue |
marc-hb
left a comment
marc-hb
left a comment
There was a problem hiding this comment.
Code changes require a very different level of review attention. So when you have clippy fixes sneaking in with much larger spell check changes, now all changes require a high-level of review attention and workload - cause you can't be sure.
The commit message and title should mention that the spell checker configuration are only usable by VSCode (and not run in CI either).
| //! File generation up to 1 GB is really fast, Benchmarking above 100 MB takes very long. | ||
|
|
||
| // clippy analyzes wrongly | ||
| #![allow(dead_code)] |
There was a problem hiding this comment.
could you please share more details?
There was a problem hiding this comment.
@sylvestre Yes, I get unused code warnings for constants and functions used in the benches. This only happens when I run "cargo clippy --workspace --all-targets --all-features" and may have to do with the benches not being used in normal coding. I have no idea how to prevent this from happening.
There was a problem hiding this comment.
seems unrelated to the spell changes
please do that in a different pr
|
@marc-hb I have no clue what you are trying to say. There are no code changes in here, so what can you not be sure? And for the spell-checker, yes, it is a VS Code extension, but I do not know if the same provider offers extensions for other IDEs which work identical. This is the same as in coreutils, therefore I did not explain the reasoning behind it. Please clarify your issue and give an example where it makes code review more complex. |
|
|
||
| /// Generate test files with these sizes in KB. | ||
| const FILE_SIZE_KILO_BYTES: [u64; 4] = [100, 1 * MB, 10 * MB, 25 * MB]; | ||
| const FILE_SIZE_KILO_BYTES: [u64; 4] = [100, MB, 10 * MB, 25 * MB]; |
There was a problem hiding this comment.
same, should be in a different pr
There was a problem hiding this comment.
okay, I sometimes have to do these changes as clippy complains. As this is not any actual change, I did not create a separate PR. But if that is easier for the reviewers, I will do better. This are then more PRs which I thought would be more work.
So I will remove the benches changes from this PR and create a separate one. No prob.
There was a problem hiding this comment.
One of the best guides on this topic in my subjective opinion:
It has been written by maintainers of a very active project.
A middle-ground is to split into separate commits inside the same PR instead of submitting separate PRs, as also described there. However, this means git commits cannot be used for addressing review comments anymore (no "fixup" commits). https://blog.buenzli.dev/announcing-development-on-flirt#the-pr-workflow So it's probably not compatible with this project
This page is also pretty good: https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes
|
Just try to put yourself in the shoes of an overworked maintainer trying to review a bag of different things unrelated to each other but mixed together. |
3 participants
For Code Spell Checker extension.
Fixed some clippy warnings.