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

Improve meson build system #6

Closed
wants to merge 3 commits into from
Closed

Improve meson build system #6

wants to merge 3 commits into from

Conversation

NathanBnm
Copy link
Contributor

Hi @casasfernando I made some cleanup to your project structure, especially with meson files.

Let me know if you have any questions.

Copy link
Owner

@casasfernando casasfernando left a comment

Choose a reason for hiding this comment

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

Thanks for this great contribution, I'm merging it now and will apply similar changes to my other projects. 🎉
The second commit I will ommit it for now as I would like to keep the translation files around.

Thanks again!

@NathanBnm
Copy link
Contributor Author

NathanBnm commented Dec 3, 2021

@casasfernando do not hesitate to create a PR on your other projects and ask me for review if you want me to check 😄

Regarding translation files. It's better not to add them for untranslated languages. Translators have to add their language code themselves into LINGUAS files and run the language files update commands to generate the associated .po files
and translate them. This way the translation files are always matching the last version. Adding a blank translation file may suggest that there is already a translation for that language

You could add a translator contributing guide inside the translations folder and link it to the appdata "Translate" link.
A good example is https://github.com/ryonakano/reco/blob/main/po/README.md

Note: The polish translation files are not properly formatted

@casasfernando
Copy link
Owner

Great explanation again. I will merge the second commit as well.
Just one question: for the polish translation in your commit I see the chaset changed from UTF-8 to ASCII, are you sure this is correct? I know that for spanish we need UTF-8 due to the special characters and I would suspect this may be the same for polish.

@casasfernando casasfernando reopened this Dec 3, 2021
@NathanBnm
Copy link
Contributor Author

Great explanation again. I will merge the second commit as well. Just one question: for the polish translation in your commit I see the chaset changed from UTF-8 to ASCII, are you sure this is correct? I know that for spanish we need UTF-8 due to the special characters and I would suspect this may be the same for polish.

You're right I didn't noticed that the charset was affected when I regenerated the file 👀
Polish contains accentuated letters so we need UTF-8 for it

@NathanBnm
Copy link
Contributor Author

Just made the fix 😉

@casasfernando
Copy link
Owner

Cool will proceed with the merge.
BTW you mentioned the polish file was not properly format, do you mind pointing me out those errors so I can learn and detect this in the future?

@NathanBnm
Copy link
Contributor Author

There was a mismatch between the .pot file and the Polish .po file headers. You can always refer to the translation template file in order to check.

Usually when you add a language as a translator you can add the language code to the LINGUAS file and use the command line (ninja com.github.casasfernando.wingpanel-indicator-weather-update-po) to generate the associated .po file.

In this case the translator didn't use this method so the file header was altered (See ee458ca).

Here is the difference after regenerating the file:

Before

msgid ""
msgstr ""
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"X-Generator: POEditor.com\n"
"Project-Id-Version: Wingpanel Weather\n"
"Language: pl\n"

After

# Polish translations for com.github.casasfernando.wingpanel-indicator-weather package.
# Copyright (C) 2021 THE com.github.casasfernando.wingpanel-indicator-weather'S COPYRIGHT HOLDER
# This file is distributed under the same license as the com.github.casasfernando.wingpanel-indicator-weather package.
# Automatically generated, 2021.
#
msgid ""
msgstr ""
"Project-Id-Version: com.github.casasfernando.wingpanel-indicator-weather\n"
"Report-Msgid-Bugs-To: \n"
"POT-Creation-Date: 2021-12-01 22:54+0100\n"
"PO-Revision-Date: 2021-12-01 22:54+0100\n"
"Last-Translator: Automatically generated\n"
"Language-Team: none\n"
"Language: pl\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"Plural-Forms: nplurals=3; plural=(n==1 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 "
"|| n%100>=20) ? 1 : 2);\n"

Translators can also manually copy the content of the translation template file but it's easiest to use the automated way which completes the file headers automatically. Usually translators just have to replace Automatically generated by their name and e-mail.

Hope I have answered to your concerns.

@NathanBnm
Copy link
Contributor Author

NathanBnm commented Dec 3, 2021

I assume this can be closed now right?

@casasfernando
Copy link
Owner

Brilliant, got it now!
If you find more things that can be improved in any of my projects, please keep the feedback coming as I'm still learning as mentioned in the README files. 😄
All changes merged, closing the PR.

Thanks.

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.

None yet

2 participants