D5133: Introduce fetchpo.rb

[Harald Sitter]

sitter requested changes to this revision.
sitter added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> fetchpo.rb:52
> +
> +elements = ReleaseMe::Project.from_xpath(options.project)
> +unless elements.count == 1

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`.

> fetchpo.rb:54
> +unless elements.count == 1
> +    warn "found #{elements.count} elements for #{options.project}"
> +    exit 2

2 space indentation for new code please.

> l10n.rb:217
>  
> -    def get(srcdir)
> -      # FIXME: this is later used as destination for the weirdest of reasons...
> -      target = "#{srcdir}/po/"
> +    def get(srcdir, target = nil, allow_edit = true)
> +      if not target

These additions are missing a test case :P

> l10n.rb:219
> +      if not target
> +          target = "#{Dir.getwd}/#{srcdir}/po"
> +      end

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)

> l10n.rb:291
>  
> -        if has_translation
> -          # Update CMakeLists.txt
> -          CMakeEditor.append_po_install_instructions!(Dir.pwd, 'po')
> -        else
> -          # Remove the empty translations directory
> -          Dir.delete('po')
> +        if allow_edit
> +            if has_translation

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)

REPOSITORY
  R572 releaseme

REVISION DETAIL
  https://phabricator.kde.org/D5133

To: apol, #frameworks, sitter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170322/a8133e8e/attachment-0001.html>