Introduce fetchpo.rb
ClosedPublic

Authored by apol on Mar 22 2017, 4:13 PM.

Details

Reviewers
sitter
Group Reviewers
Frameworks
Commits
R572:474c8704ce76: Introduce fetchpo.rb
Summary

Adds an executable that will allow to retrieve translations for a checked out
project.

Test Plan

tests still pass

Diff Detail

Repository
R572 releaseme
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Mar 22 2017, 4:13 PM
sitter requested changes to this revision.Mar 22 2017, 4:50 PM

I really think this should have a simple test case. In particular, the allow_edit behavior should be asserted somewhere. Also some style nitpicks.

fetchpo.rb
53

Unless you want this to work even for !git builds I'd really advise that you get attempt to find the git remote URL and use Project.from_repo_url to resolve that into a Project. from_xpath is slower and less reliable than from_repo_url.

55

2 space indentation for new code please.

lib/releaseme/l10n.rb
217

These additions are missing a test case :P

219

Bunch of style problems here. TLDR code I want at the bottom.

Dir.pwd + modifier if (when the line doesn't exceed 80 chars) + unless is preferred for style reasons (https://github.com/bbatsov/ruby-style-guide)

i.e.

target = "#{Dir.pwd}/#{srcdir}/po" unless target

That said, instead of expanding with Dir.pwd manually you'd want to File.expand_path("#{srcdir}/po"). If srcdir is already an absolute path it will append /po, if it is not it will be turned into an absolute path relative to pwd whilest also expanding ~ correctly http://ruby-doc.org/core-2.4.0/File.html#method-c-expand_path

Also, in ruby, method params are evaluated as they appear, so since target is after srcdir you can already deref srcdir.

Putting everything together this can be expressed in the method definition without any if:

def get(srcdir, target = File.expand_path("#{srcdir}/po"),
        allow_edit = true)
293

I think the ifs here need to be moved inside out.

if has_translations
   cmakeappendcrap if allow_edit
else
   Dir.delete('po')
end

(allow_edit shouldn't have an impact on cleaning up the dangling po dir)

This revision now requires changes to proceed.Mar 22 2017, 4:50 PM
apol updated this revision to Diff 12695.Mar 22 2017, 5:38 PM
apol edited edge metadata.

Address sitter concerns

sitter requested changes to this revision.Mar 22 2017, 5:43 PM
sitter added inline comments.
lib/releaseme/l10n.rb
217

File.expand_path

def get(srcdir, target = File.expand_path("#{srcdir}/po"),
            allow_edit = true)
This revision now requires changes to proceed.Mar 22 2017, 5:43 PM
apol updated this revision to Diff 12696.Mar 22 2017, 6:26 PM
apol edited edge metadata.

Address issue, use git.kde.org repo name as identifier

sitter accepted this revision.Mar 22 2017, 6:54 PM
This revision is now accepted and ready to land.Mar 22 2017, 6:54 PM
This revision was automatically updated to reflect the committed changes.