Refactor/switch to american english#830
Conversation
|
Btw, what script did you use? Thinking about the future use... What do you think, should this be automated like "Style code"? |
|
I just used https://www.npmjs.com/package/american-british-english-translator to look see what spelling needs to be changed, and then just used something like for each word: We can use a bash script to do this automatically at every PR. |
|
Don't forget to fix the permissions for styler: https://github.com/microbiome/OMA/actions/runs/21026866334/job/60453346460 |
It is working for OMA branches, but the styler does not have access to forks. It might be possible somehow to add them as " Maintainers are allowed to edit this pull request. ", but don't know how. Can you check? I.e., can styler have "maintainer" status? |
Can you make one? Create new GHA for that so we can remove it if it is not working |
|
Note: some words do not have to be substituted, such as |
|
Should be fine now. Let me know, and I wish squash the commits. |
|
It seems that the GHA is failing to push. "Style" GHA is also pushing --> can you check that these both are using same approach |
This avoids checking out the repo in a detached state. See https://github.com/stefanzweifel/git-auto-commit-action?tab=readme-ov-file#checkout-the-correct-branch for more info.
|
That's expected because the But I will see what I can do to fix it. |
|
Now the conversion GHA is fixed. The style fails because of the 403 error (access forbidden). I'm not entirely sure what's going on here because I gave it the write permissions to the repo. However, I plan to switch to this Action for commit and push the changes. Right now, it's my custom script. |
| -e 's/visualise/visualize/' \ | ||
| -e 's/visualisation/visualization/' \ | ||
| -e 's/summarise /summarize /' \ | ||
| -e 's/summarised /summarized /' \ |
There was a problem hiding this comment.
Why there is space after summarised? Does it mean that this does not work for
This sentence is summarised.
There was a problem hiding this comment.
Why there is space after summarised? Does it mean that this does not work for
This sentence is summarised.
The reason is that we have a summarise function, so summarise() would be
matched. That's unwanted behavior. I would use the negated set here
(https://www.regular-expressions.info/charclass.html).
I tested it briefly: it will not work with summarise (no space character),
but I suppose this situation should not occur.
So, something like this: "s/<summarise>([^(])/summarize\1/" and
"s/<summarised\>([^(])/summarized\1/". Not sure if this can be merged into
one rule. Need some Regex gurus here.
There was a problem hiding this comment.
For now, can you move the GHA related stuff to the PR: #840
The text is good to go but this might need more thinking?
There was a problem hiding this comment.
I still feel this is not safe enough as it can match with function names or variables and lead to problems in the future. Maybe words inside code chunks should be ignored, but I would already merge the qmd files from this PR to get forward.
There was a problem hiding this comment.
We can use the negation pattern ^ to ignore some blocks, but I feel like
we are entering the bash script area here. Do you know someone with better
regex knowledge?
There was a problem hiding this comment.
You might be the best candidate that I know. Also, this might need more sophisticated approach (for instance dedicated R function).
As the PR is good otherwise, I propose that the GHA related stuff will be moved to another PR
|
Thanks, looks good! Check the last comment. |
2 participants
See #800.
I think I covered all of British spelling.