Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented May 3, 2017

Fixes #597

This pull request seeks to ensure consistent output of the languages/gutenberg.pot file between Windows and Unix by forcing the relative path string references to use the POSIX implementation of Node's path module.

Testing instructions:

  1. Run npm run gettext-strings
  2. View languages/gutenberg.pot
  3. Note that file references use / slash, not \

@BE-Webdesign, would you mind taking this for a spin on Windows?

Note: As of #614, the .pot file is now ignored from the Git repository, so you shouldn't expect to see any changes reported by git status.

@aduth aduth added [Type] Build Tooling Issues or PRs related to build tooling Internationalization (i18n) Issues or PRs related to internationalization efforts labels May 3, 2017
@aduth aduth requested a review from BE-Webdesign May 3, 2017 13:36
@BE-Webdesign
Copy link
Contributor

This is better but I think the process.cwd() is still in Windows style, so when the path is made relative it doesn't work correctly.

I got it to work by doing something like this:

{ relative } = require( 'path' ).posix;
{ sep } = require( 'path' );
// Do the above however you want so that the module is not required twice.

// For the comments part
relative( process.cwd().split( sep ).join( '/' ), filename ) + ':' + path.node.loc.start.line

I don't know if that works on *nix. I don't have node installed on my VM.

Copy link
Contributor

@BE-Webdesign BE-Webdesign left a comment

Choose a reason for hiding this comment

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

At line 205ish relative( process.cwd().split( sep ).join( '/' ), filename ) + ':' + path.node.loc.start.line. Should be used where sep is taken from the base path module so it will match either a Windows separator or a POSIX separator.

@nylen
Copy link
Member

nylen commented May 4, 2017

Given that the .pot file no longer exists in the repository after #614, why do we need to do this at all?

@aduth
Copy link
Member Author

aduth commented May 4, 2017

Given that the .pot file no longer exists in the repository after #614, why do we need to do this at all?

Since we'll still need to generate strings, I think there's value in ensuring that the output is consistent across OS's (OSes? OSs? Grammar 😩 ).

@BE-Webdesign
Copy link
Contributor

The only benefit will come from someone who wants to help develop, work on translations and happens to use Windows.

@nylen
Copy link
Member

nylen commented May 4, 2017

Pretty sure the correct spelling is OS\es. :trollface:

@aduth
Copy link
Member Author

aduth commented May 4, 2017

@BE-Webdesign I've taken another stab at this. I found process.cwd() could simply be replaced with '.' .

In my testing, there's still a minor issue where if the project is cloned in "My Documents", the filename passed from Babel starts with C:/Users/<user>/Documents, but process.cwd() evaluates to C:\\Users\\<user>\\My Documents, resulting in ../../Documents/ prefixes to files in the generated POT file. I think we'll want to investigate this, but I'm not considering it high enough priority for now to spend much more time on.

@BE-Webdesign
Copy link
Contributor

I've taken another stab at this. I found process.cwd() could simply be replaced with '.' .

Ah, that is a great idea.

In my testing, there's still a minor issue where if the project is cloned in "My Documents", the filename passed from Babel starts with C:/Users//Documents, but process.cwd() evaluates to C:\Users\\My Documents, resulting in ../../Documents/ prefixes to files in the generated POT file. I think we'll want to investigate this, but I'm not considering it high enough priority for now to spend much more time on.

That is super edge-casey and I don't think warrants any fix really. You can also mess with your user path a lot on windows so I wouldn't worry about this.

@aduth
Copy link
Member Author

aduth commented May 4, 2017

Thanks for taking a look @BE-Webdesign !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Build Tooling Issues or PRs related to build tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants