Skip to content
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

Grammar/Spelling #38006

Merged
merged 7 commits into from
Feb 22, 2020
Merged

Grammar/Spelling #38006

merged 7 commits into from
Feb 22, 2020

Conversation

klorpa
Copy link
Contributor

@klorpa klorpa commented Feb 14, 2020

Summary

SUMMARY: Bugfixes "Fixes various punctuation and spelling errors."

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

@klorpa klorpa requested a review from I-am-Erk as a code owner February 14, 2020 01:24
@I-am-Erk
Copy link
Member

You appear to have deleted all your changes with that last update.

@anothersimulacrum
Copy link
Member

You should never edit .po files, any string edits should be in data/ or src/

Copy link
Member

@I-am-Erk I-am-Erk left a comment

Choose a reason for hiding this comment

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

I'm not sure there's value in changing comments to remove regional spelling.

Why so much editing to .po files? Does it have any actual effect

@ZhilkinSerg ZhilkinSerg added the [JSON] Changes (can be) made in JSON label Feb 14, 2020
@Qrox
Copy link
Contributor

Qrox commented Feb 15, 2020

You should see lang/po/_NO_MANUAL_EDITING for why you shouldn't edit the .po files.

Given how many times we've been repeating this, we should probably add it as an automated check to warn about it to unknowing contributers.

@I-am-Erk
Copy link
Member

I don't think an automated check would fix things since they're already in the PR stage by then

@Qrox
Copy link
Contributor

Qrox commented Feb 17, 2020

But the author would see the warning and revert the changes before we have to tell them to do so. (Well at least if they are paying attention to the check status)

@ZhilkinSerg
Copy link
Contributor

But the author would see the warning and revert the changes before we have to tell them to do so. (Well at least if they are paying attention to the check status)

Wouldn't it affect expected i18n PRs from @BrettDong? Or we would need to add some kind of whitelist for PR author to the check?

@ZhilkinSerg ZhilkinSerg merged commit 9a78ab5 into CleverRaven:master Feb 22, 2020
Soupster89 added a commit to Soupster89/Cataclysm-DDA that referenced this pull request Feb 24, 2020
@Qrox
Copy link
Contributor

Qrox commented Feb 24, 2020

@ZhilkinSerg If a PR is an authentic i18n update we can just ignore the warning and merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants