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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to show the relative paths in the .pot comments #12438

Merged
merged 2 commits into from Aug 10, 2018

Conversation

Projects
None yet
5 participants
@Spriz
Copy link
Contributor

Spriz commented Aug 7, 2018

This change makes is possible to put in a relative path in the .pot file generated by the ExtractTask used by the I18nShell.

Without using this new argument:

#: /var/lib/jenkins/jobs/apacta-translations-update/workspace/plugins/Companies/src/Template/Projects/add.ctp:425

With this new argumentthis would be

#: ./plugins/Companies/src/Template/Projects/add.ctp:425

Not using this argument creates a lot of diffs when this file is sometimes updated through CI server, sometimes by developers or maybe by a 3rd party tool that doesn't quite understand these paths anyway - at least the full path doesn't make a lot of sense to either us or our tools :-)

I don't see a lot of tests covering the comments in the file, and my regex is not strong - so maybe someone would like to help a bit? 馃憤

@Spriz Spriz changed the title Option to show the relative paths in the .pot Option to show the relative paths in the .pot comments Aug 7, 2018

@dereuromark dereuromark added this to the 3.7.0 milestone Aug 7, 2018

@@ -447,6 +461,9 @@ protected function _parse($functionName, $map)
'file' => $this->_file,
'line' => $line,
];
if ($this->_relativePaths) {
$details['file'] = '.' . str_replace(ROOT, '', $details['file']);

This comment has been minimized.

@Spriz

Spriz Aug 7, 2018

Author Contributor

If anyone has a better idea for the string replacing here - please just change it!

Also using the . in front of the path is more of a personal experience. I couldn't see Cakephp using either ./ or / so I just picked my favorite.

Just tell me if I should change it.

This comment has been minimized.

@ADmad

ADmad Aug 7, 2018

Member

I personally would just use foo/bar/baz.ctp without any leading dot or slash.

This comment has been minimized.

@dereuromark

dereuromark Aug 7, 2018

Member

I usually replace ROOT . DS completely :)
But either way is already relative and works fine.
So this looks already good!

@Spriz Spriz force-pushed the Spriz:feature/RelativePathsInPotComments branch from 92ff2bd to ded811d Aug 7, 2018

@Spriz

This comment has been minimized.

Copy link
Contributor Author

Spriz commented Aug 7, 2018

Just squashed 3 commits to a single.

@ADmad
Copy link
Member

ADmad left a comment

LGTM, needs tests.

@@ -447,6 +461,9 @@ protected function _parse($functionName, $map)
'file' => $this->_file,
'line' => $line,
];
if ($this->_relativePaths) {
$details['file'] = '.' . str_replace(ROOT, '', $details['file']);

This comment has been minimized.

@ADmad

ADmad Aug 7, 2018

Member

I personally would just use foo/bar/baz.ctp without any leading dot or slash.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 7, 2018

Codecov Report

Merging #12438 into 3.next will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #12438      +/-   ##
============================================
+ Coverage      91.8%    91.8%   +<.01%     
  Complexity    13835    13835              
============================================
  Files           517      517              
  Lines         35588    35589       +1     
============================================
+ Hits          32673    32674       +1     
  Misses         2915     2915
Impacted Files Coverage 螖 Complexity 螖
src/Shell/Task/ExtractTask.php 76.92% <100%> (+0.4%) 118 <0> (+2) 猬嗭笍
src/View/View.php 86.61% <0%> (-0.14%) 170% <0%> (-2%)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 51f46ef...cef07d3. Read the comment docs.

@@ -233,6 +240,10 @@ public function main()
$this->_merge = strtolower($response) === 'y';
}
if (isset($this->params['relative-paths'])) {

This comment has been minimized.

@markstory

markstory Aug 7, 2018

Member

You could use if (!$this->param('relative-paths')) { here.

@@ -321,6 +332,9 @@ public function getOptionParser()
])->addOption('merge', [
'help' => 'Merge all domain strings into the default.po file.',
'choices' => ['yes', 'no']
])->addOption('relative-paths', [
'help' => 'Use relative paths in the .pot file',
'choices' => ['yes', 'no']

This comment has been minimized.

@markstory

markstory Aug 7, 2018

Member

This could be a boolean argument. That will default it to false and when --relative-paths is set on the CLI it will be true.

@markstory markstory self-assigned this Aug 7, 2018

@markstory markstory added the Need Tests label Aug 7, 2018

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Aug 7, 2018

I can help with tests.

@garas

This comment has been minimized.

Copy link
Member

garas commented Aug 7, 2018

This PR duplicates what --paths param does.

By default ExtractTask strips /fullpath/src/ (APP const)

If you add --extract-core, it also strips /fullpath/vendor/cakephp/cakephp/src/ (CAKE const)

You can add as many paths as you wish with --paths=/fullpath/src/,/fullpath/.

Or use --plugin=Companies

@ADmad

This comment has been minimized.

Copy link
Member

ADmad commented Aug 7, 2018

@garas This new option is lot more intuitive :)

@garas

This comment has been minimized.

Copy link
Member

garas commented Aug 7, 2018

Then this parameter should only control whether this str_replace is applied.

if (!$this->param('no-location')) {
$header = '#: ' . str_replace(DIRECTORY_SEPARATOR, '/', str_replace($paths, '', $occurrences)) . "\n";
}

Also maybe add ROOT to default list of paths, but that would slowdown if app has many composer dependencies.

@markstory markstory removed the Need Tests label Aug 9, 2018

@markstory markstory merged commit 25838c4 into cakephp:3.next Aug 10, 2018

5 checks passed

codecov/patch 100% of diff hit (target 91.8%)
Details
codecov/project 91.8% (+<.01%) compared to 51f46ef
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

markstory added a commit to cakephp/docs that referenced this pull request Aug 10, 2018

@Spriz Spriz deleted the Spriz:feature/RelativePathsInPotComments branch Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.