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

Keep custom flags with the custom_flags_to_keep option #370

Merged
merged 2 commits into from
Jul 31, 2023

Conversation

johantell
Copy link
Contributor

Background:
While merging translations with mix gettext.merge flags get cleared out and replaced with the exception of fuzzy, which is kept.

This change will allow the custom_flags_to_keep flag to add a list of flags to be kept in addition to fuzzy which helps prevent information loss if external tools adds their own flags to messages.

Fixes #369

Background:
While merging translations with `mix gettext.merge`
flags get cleared out and replaced with the exception of
`fuzzy`, which is kept.

This change will allow the `custom_flags_to_keep` flag to
add a list of flags to be kept in addition to `fuzzy` which
helps prevent information loss if external tools adds their
own flags to messages.

Fixes elixir-gettext#369
@johantell
Copy link
Contributor Author

It got a little bit awkward to pass on the gettext_config as deep down as I had to, but I'll let you decide if it's OK or not :)

@whatyouhide
Copy link
Contributor

@johantell I’m seeing that we don't really have any Gettext-level config related to merging, right? Because if so, we could potentially start with making this a CLI flag for the gettext.merge task. However, I recognize that it would be easier to configure this on the project level so that contributors to a project don't have to remember to pass the flag. On the third hand, Mix aliases... Thoughts? 😄

@johantell
Copy link
Contributor Author

@whatyouhide I would prefer it to be a config option rather than a CLI flag, primarily because I've heard from colleagues that they sometimes use the mix alias we have and sometimes not.

I somehow see this more as a global-level option than something that would tend to change between runs, whereas the only CLI options related to merging (from what I can understand) are locale and no-fuzzy.

I could see how merging the configured options with the CLI input into a single struct could make the number of arguments go down, but I would also assume that to be a significant undertaking only to have fewer arguments passed around :P.

lib/gettext.ex Outdated Show resolved Hide resolved
test/gettext/merger_test.exs Outdated Show resolved Hide resolved
@whatyouhide whatyouhide merged commit d7e1bef into elixir-gettext:main Jul 31, 2023
@whatyouhide
Copy link
Contributor

Thank you @johantell! ❤️

else
new.flags
end
@default_flags_to_keep ["elixir-format", "fuzzy"]
Copy link
Member

Choose a reason for hiding this comment

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

We should only have a special rule for the fuzzy flag. The format flag is depending on the enabled interpolation module and might differ from elixir-format.

Copy link
Member

Choose a reason for hiding this comment

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

Too late for merge, please still consider it @whatyouhide 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

@maennchen ah that's a great call out. PR? 😬

Copy link
Member

Choose a reason for hiding this comment

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

@whatyouhide I recently had surgery and am on a lot of pain meds. I think it would be better if you could do this without me…

Copy link
Contributor

Choose a reason for hiding this comment

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

@maennchen ah, sure! Good luck with recovery then 🤗

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! ✔️

@johantell johantell deleted the keep_custom_flags_option branch August 7, 2023 14:47
@johantell
Copy link
Contributor Author

@whatyouhide Do you think there any chance we'll see a release that includes these changes anytime soon, or would you prefer to wait a bit to include other things in the next release? 👼

@whatyouhide
Copy link
Contributor

@johantell I don't have cycles to spend on this right now, so for now no release. I can try to do it by end of week, but we'll see how things look like. Sorry!

@johantell
Copy link
Contributor Author

@whatyouhide Alright, no problem. I'll lock on the latest commit until you've found time. Thanks for taking the time to answer, and let me know if there is anything I can do to help out :)

@whatyouhide
Copy link
Contributor

@johantell ok I snuck this in, 0.23.0 is out! 👍

@johantell
Copy link
Contributor Author

@whatyouhide You're my hero! ❤️ 💛 💚 💙

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.

Retain custom flags during merge
3 participants