Closed yarikoptic closed 1 year ago
I would be very reluctant to introduce a spell checker for the code.
Comments and .md files are one thing that makes sense to check, but changing variable names within the code will likely produce undesirable effects like increasing line lengths in some scenarios (which would violate Rubocop rules and code readability) and potentially renaming a variable within a nested block so that it matches a variable declared outside the block.
For example something like:
plan = Plan.find_by(id: params[:id])
array_of_other_plans.each do |pln|
result = do_something(pln)
plan.do_another_thing if result.valid?
end
Would get changed to
plan = Plan.find_by(id: params[:id])
# 'pln' renamed to 'plan' can cause confusion here
array_of_other_plans.each do |plan|
result = do_something(plan)
plan.do_another_thing if result.valid?
end
I do understand the hesitance ;) note that
codespell
is primarily the checker and that is how it is used here in the workflow, that it would not change the code "automagically". Indeed if the new code introduces some odd short variable name, its CI run can fail and require code tune up.
.codecovrc
pln
and plan
(in your example) in the same nearby code. At least I've done a number of times. So IMHO it could even be a bad practice to have such "close calls".recipientname
) so I was "greedy" in replacing that hit everywhere to not miss any usage
Having said that - indeed this PR first should be carefully reviewed to not skip any code alternation which could have had undesired effect. I did check as carefully as I could but I'm not a ruby developer.
So, overall - it is all up to you: can analyze and express desires on what to tune up, can ignore and close and carry on as it never happened (thus with all the typos), can do something in the middle of that spectrum ;) I just didn't want to pass by whenever spotted a respectful number of hits.
thanks. I'll have a look in more detail to assess the proposed changes. Some of our test coverage isn't what it should be, so I'm always a bit paranoid when I see so many files updated ;)
Don't you worry, I have plenty of similar paranoidal changefobias, so "I hear you" and would be happy to see if you find anything I missed while going through the diff ;)
nevertheless I would like to ping on your opinion on this PR before I jump to address conflicts -- the longer it stays undecided, the more effort should be invested to make dmptool gloriously-free-of-typos.
@yarikoptic can you modify the codepsellrc file to skip files in config/locale/**/*
and app/javascript/src/locale/**/*
when you have some time?
Those files are auto-generated by Translation.io for us so modifying in this way circumvents that workflow. The strings contained in those files are extracted from the rest of the codebase and then pushed up to Translation.io where our partners translate new strings. Since codespell will fix the typos in the other files, the corrections will eventually propagate into the locale files.
done. FWIW, I liked how I managed to redo the RUNCMD automated ones I did via datalad-run, I just created following git-sedi
which excluded /locale/
paths and then used datalad rerun
which reapplied those sed
invocations. worked out neatly ;) in other commits where I did do datalad run
, had to resort to some manual interactive rebasing and reworking to cancel changes to /locale/
folders.
@briri -- what we will do with this PR since I would be able to keep readjusting forever.
how much will you be able to review to have that portion merged? I could go "piece meal" way
apologies @yarikoptic I am a team of one at the moment and just do not have the bandwidth for this one.
You might consider contributing to the DMPRoadmap parent repository though as it will reach a broader audience
git grep
and then applyingsed
(see commit msgs)