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

Update 'release' to work with po4a #510

Merged
merged 19 commits into from
Jun 5, 2021
Merged

Update 'release' to work with po4a #510

merged 19 commits into from
Jun 5, 2021

Conversation

ignotus666
Copy link
Contributor

Does this need translation?

  • Yes
  • [* ] No

Changes

This PR will update 'release' to work with po4a. All wiki .md files are unchanged save for the removed en- prefixes. I built the website locally and everything seems to work properly.

@ignotus666
Copy link
Contributor Author

I've fixed several anchors in FR and EN, a few typos... I'm going to leave it there. Aside from whatever needs to be updated from now on, I think it's all ship-shape in its current incarnation.

@ann0see ann0see requested review from ann0see and jujudusud June 4, 2021 11:44
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Not yet approving since I plan to do a deeper review in future.


For translating text in the Jamulus application itself, please see [TRANSLATING.md](https://github.com/jamulussoftware/jamulus/blob/master/TRANSLATING.md)

We collect changes to the English version of the site on a "changes" branch first. We then freeze changes prior to a Jamulus software release, and do a translation "sprint" over a couple of weeks when all translation takes place.
We collect changes to the English version of the site on a "next-release" branch first. We then freeze changes prior to a Jamulus software release, and do a translation "sprint" over a couple of weeks when all translation takes place.
Copy link
Member

Choose a reason for hiding this comment

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

This needs renaming of the changes branch --> todo

README.md Outdated

### Here’s the overall workflow

1. Changes are first made to en-*.md files and committed to the “changes” branch.
1. Changes are first made to EN *.md files and committed to the “next-release” branch.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Changes are first made to EN *.md files and committed to the “next-release” branch.
1. Changes are first made to EN (= English) *.md files and committed to the “next-release” branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -62,6 +63,8 @@ We collect changes to the English version of the site on a "changes" branch firs

## Adding a new language

First open an issue so that the relevant folders and files can be created for your language in `translator-files/l10n/po/`.
Copy link
Member

Choose a reason for hiding this comment

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

Adding a new language should be documented somewhere else, so that more people know what needs to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where to add it. Where do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Either a separate file or here.

po4a-create-all-targets.sh Show resolved Hide resolved
po4a-create-all-targets.sh Outdated Show resolved Hide resolved
po4a-create-all-targets.sh Outdated Show resolved Hide resolved
####################################

cd ./wiki/
rm -rf de es fr it pt
Copy link
Member

Choose a reason for hiding this comment

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

Could you use an external config file or even better check which languages exist based on _config.yml? Hardcoding means we need to remember changing this every time a language gets added or deleted.

Copy link
Contributor Author

@ignotus666 ignotus666 Jun 4, 2021

Choose a reason for hiding this comment

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

I had a go at extracting the dir names from PO_DIR to use as a variable in a for loop, as those will be the supported languages, but I couldn't get it to work and don't really want to spend ages figuring it out. Bear in mind that I took most of the content of these scripts from elsewhere and adapted them. It's a miracle I got them to work at all... I have zero experience writing scripts beyond making simple app launchers for my desktop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Success. Might be hacky but it works.

po4a-create-all-targets.sh Outdated Show resolved Hide resolved
po4a-create-all-targets.sh Outdated Show resolved Hide resolved
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Thanks again! Once I have more time, I'll do a deeper review; but from a first glance I'd like to have more comments (for each language: delete folders or check if directory exists/run po4a to generate md files for jekyll,...)

@ann0see ann0see linked an issue Jun 4, 2021 that may be closed by this pull request
@ann0see ann0see merged commit 3cf2089 into jamulussoftware:release Jun 5, 2021
@ann0see
Copy link
Member

ann0see commented Jun 5, 2021

Merged; however it still needs some more work probably.

path="${dirname#$SRCDIR_MODULE/}"

if [ "$dirname" = "$SRCDIR_MODULE" ]; then
potname=$basename.pot
Copy link
Member

Choose a reason for hiding this comment

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

@ignotus666 I'm a bit worried about quoting here. If $basename includes spaces it might break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if [ "$dirname" = "$SRCDIR_MODULE" ]; then
potname=$basename.pot
else
potname=$path/$basename.pot
Copy link
Member

Choose a reason for hiding this comment

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

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +6 to +8
# SRCDIR root of the documentation repository
# PODIR place where to create the po
# PUB_DIR place where to publish the localised files
Copy link
Member

Choose a reason for hiding this comment

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

You should put this directly at the lines where you define these variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been done in a previous PR. You seem to be checking the version in "next-release". The one in "release" has those comments in place.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Ok. Sorry about that. I reviewed this (merged) PR which didn't include the latest changes you did.

do
lang=$(basename -s .md "$dir")
echo "delete $lang folder"
cd ./wiki/
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The indentation seems correct if I open it as a text file locally... I'll have a look.


# Check if po4a is installed
if ! [ -x "$(command -v po4a)" ] ; then
echo 'Error: please install po4a.' >&2
Copy link
Member

Choose a reason for hiding this comment

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

Consistency: please use double quotes here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

do
basename="$(basename -s .md "$file")"
dirname=$(dirname "$file")
path="${dirname#$SRCDIR_MODULE/}"
Copy link
Member

Choose a reason for hiding this comment

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

Not totally sure what this one does here. Could you please explain? # seems to count how long a variable is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it takes the variable 'dirname' and assigns what's after SRCDIR_MODULE (this variable name has changed in the new version) to it. I'm not sure though... those lines were pilfered from elsewhere...

Comment on lines +6 to +7
# SRCDIR root of the documentation repository
# POTDIR place where to create the .pot templates
Copy link
Member

Choose a reason for hiding this comment

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

Same as in po4a-create-all-targets.sh


# Check if po4a is installed
if ! [ -x "$(command -v po4a)" ] ; then
echo 'Error: please install po4a.' >&2
Copy link
Member

Choose a reason for hiding this comment

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

Quoting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

po_file="$PO_DIR/$lang/${potname%.pot}.po"

# po4a-updatepo would be angry otherwise
sed -i 's/Content-Type: text\/plain; charset=CHARSET/Content-Type: text\/plain; charset=UTF-8/g' "$po_file"
Copy link
Member

Choose a reason for hiding this comment

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

Probably a bit hacky, but seems to work.

Copy link
Contributor Author

@ignotus666 ignotus666 left a comment

Choose a reason for hiding this comment

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

If you have more comments go ahead - I'll make PR tonight. No time now.

@ann0see
Copy link
Member

ann0see commented Jun 6, 2021

No problem;-)

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.

Create po4a branch for changes/corrections to EN and translations
3 participants