Skip to content

Conversation

yarikoptic
Copy link

@yarikoptic yarikoptic commented Jan 26, 2023

  • individual commit messages would give more detail
  • at large localization was left without fixes (and excluded from codespell'ing) but some common typos were fixed through global matching using git grep and then applying sed (see commit msgs)
    • might have been too enthusiastic -- modified binary .mo files as well. Anyways probably should be regenerated -- is there a 'standard workflow'? (made it into checkmark so does not go unnoticed)

@briri
Copy link
Collaborator

briri commented Jan 26, 2023

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

@yarikoptic
Copy link
Author

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.
  • indeed it can pick up on some abbreviated variables which are close to real words. But hey -- humans are close to computers in making mistakes whenever they see both 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".
  • here some fixes "in code" were "tricky" in that they had to be manifested also elsewhere via templates (e.g. that recipientname) so I was "greedy" in replacing that hit everywhere to not miss any usage
    • needless to say that carrying forward while relying on mistyped variable name could have its own gotchas and difficulty of contributing knowing on how to mistype every time ;)

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.

@briri
Copy link
Collaborator

briri commented Jan 27, 2023

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 ;)

@yarikoptic
Copy link
Author

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 ;)

@yarikoptic
Copy link
Author

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.

@briri
Copy link
Collaborator

briri commented Mar 24, 2023

@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.

…ran across entire repo

reworked while unapplying locale fixes
=== Do not change lines below ===
{
 "chain": [
  "3bf9ab10b3f6a00cd47b55f190d9ab6490f86fda"
 ],
 "cmd": "git sedi requirments requirements",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [
  "663a7bd399e659602ca2cff5c9fafe64f10fff8f"
 ],
 "cmd": "git sedi recepient recipient",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@yarikoptic
Copy link
Author

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 briri self-assigned this Apr 12, 2023
@yarikoptic
Copy link
Author

@briri -- what we will do with this PR since I would be able to keep readjusting forever.

qs = @plan.questions.select { |q| q.themes.collect(&:title).include?(theme) }
descr = qs.map do |q|
a = @plan.answers.select { |ans| ans.question_id = q.id }.first
a = @plan.answers.select { |answer| answer.question_id = q.id }.first
Copy link
Author

Choose a reason for hiding this comment

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

ping on this one -- is this legit change or should I keep ans here? I do not know ruby

@yarikoptic
Copy link
Author

how much will you be able to review to have that portion merged? I could go "piece meal" way

@briri
Copy link
Collaborator

briri commented Oct 12, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants