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

Consolidate all translation config #3750

Merged

Conversation

alanmcruickshank
Copy link
Contributor

Fixes #3745

Move all translation config to superset/translations, as suggested in the above issue. The babel folder which has been removed was being used as a default, hence the update to the docs to show that this now needs to be specified explicitly.

Move all translation config to superset/translations
@coveralls
Copy link

coveralls commented Nov 1, 2017

Coverage Status

Coverage remained the same at 71.271% when pulling dbd6a0a on alanmcruickshank:consolidate_translations into 5bc734b on apache:master.

If you get errors running `po2json`, you might be running the ubuntu package with the same
name rather than the nodejs package (they have a different format for the arguments). You
need to be running the nodejs version, and so if there is a conflict you may need to point
directly at `/usr/local/bin/po2json` rather than just `po2json`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The path would be valid only if you installed with npm install -g. The package in debian / ubuntu looks the correct one to latest release 0.4.5 as we have in package.json. What was the error?

Copy link
Contributor Author

@alanmcruickshank alanmcruickshank Nov 1, 2017

Choose a reason for hiding this comment

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

I did install using npm install -g, as that's what it currently recommends in CONTRIBUTING.md. Should we update that to not recommend a global install?

I wonder if actually, I should update the docs here to add clarity around what needs to be done to makes sure po2json is installed in the right place more broadly. I found the process quite fiddly for someone who doesn't use node very often - global vs local installs and what folder things are available in was at the root of that I think. I'd love to make the process as easy as:

  1. Run command X in folder Y to install Z.
  2. Run command A in folder B to convert messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

It docs use global install then it's fine.

@mistercrunch
Copy link
Member

Thanks for adjusting this. Eventually we can wrap those in npm and superset CLI commands. Though these operations are not very common so it's not too important.

@mistercrunch mistercrunch merged commit 87b6d76 into apache:master Nov 2, 2017
@alanmcruickshank alanmcruickshank deleted the consolidate_translations branch November 8, 2017 16:25
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
Move all translation config to superset/translations
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.5 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up translations
4 participants