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

fixes for FPM::Package#to_s and relatives #1093

Merged
merged 3 commits into from Apr 19, 2016

Conversation

Projects
None yet
3 participants
@ptomulik
Contributor

ptomulik commented Apr 15, 2016

The main issue that this patch tries to address it that freebsd packager ignored --package CLI option. Working on it I also decided to add some customization points to FPM::Package#to_s such that subclasses can easily overload certain bits of its behavior.

@ptomulik ptomulik force-pushed the ptomulik:fix/freebsd-to_s branch from a22abfa to 478e92f Apr 15, 2016

def to_s_name; name.to_s; end
def to_s_fullversion
ver = to_s_version
ver += "-#{iteration}" if iteration

This comment has been minimized.

@jordansissel

jordansissel Apr 16, 2016

Owner

If I'm understanding your proposal, shouldn't this use "-#{to_s_iteration}" ?

This comment has been minimized.

@ptomulik

ptomulik Apr 16, 2016

Contributor

Maybe. Or, should we leave it as is and use type.to_s in the to_s_extension below? Maybe this second approach would be better, as it does not introduce "unexpected" interdependences between to_s_* functions?

def to_s_fullversion()
# iteration (PORTREVISION on FreeBSD) shall be appended only(?) if non-zero.
# https://www.freebsd.org/doc/en/books/porters-handbook/makefile-naming.html
return "#{to_s_version}_#{iteration}" if iteration and (iteration.to_i > 0)

This comment has been minimized.

@jordansissel

jordansissel Apr 16, 2016

Owner

should this also use #{to_s_iteration} ?

This comment has been minimized.

@ptomulik

ptomulik Apr 16, 2016

Contributor

Or replace to_s_version with just version (to avoid interdependencies between to_s_* methods).

@ptomulik

This comment has been minimized.

Contributor

ptomulik commented Apr 16, 2016

Corrected to_s_* stuff.

@hatt

This comment has been minimized.

Contributor

hatt commented Apr 17, 2016

Looks good!

@@ -905,7 +905,7 @@ def mkdir_p(dir)
def to_s(format=nil)
# Default format if nil
# git_1.7.9.3-1_amd64.deb
return super("NAME_FULLVERSION_ARCH.TYPE") if format.nil?
return super("NAME_FULLVERSION_ARCH.EXTENSION") if format.nil?

This comment has been minimized.

@ptomulik

ptomulik Apr 18, 2016

Contributor

I wonder why ruby people don't use the ternary if control expression? This line and line below could be simply reduced to:

super(format.nil? ? "NAME_FULLVERSION_ARCH.EXTENSION" : format)

Similarly in other modules.

This comment has been minimized.

@hatt

hatt Apr 18, 2016

Contributor

I think for the most part it's good but some lines have long gsub conditions which could make it hard to read. Re merging, only Jordan can do that.

This comment has been minimized.

@jordansissel

jordansissel Apr 19, 2016

Owner

I wonder why ruby people don't use the ternary if control expression?

I'm not what I would consider a "ruby" person (truthfully, I'm not quite sure what that means, so maybe I am! hehe). I have a habit of avoiding ternary operator, but I'm not sure why I do. ;P

This comment has been minimized.

@ptomulik

ptomulik Apr 19, 2016

Contributor

Don't mistake "ruby" with "rude". :) I needed two iterations to discover why there are two return instructions one after the another. That was crazy, as I'm not used to read whole lines of code, only first one or two words :)

This comment has been minimized.

@jordansissel

jordansissel Apr 19, 2016

Owner

haha, I agree that this is more than a bit awkward. It certainly looks like one of those "code smell" type of problems where it just feels weird. We could rewrite it like this, using default argument:

def to_s(format="NAME_FULLVERSION_ARCH.EXTENSION")
  super(format)
end

And then I wonder why this method exists just to have a default value in a weird way that only really calls super(...). We could probably refactor this to have each package type be able to describe it's own default format, then not need to define to_s for each package type.

A refactor for another day, perhaps ;P

(If you dive into doing this, I support it, happy to review whatever)

This comment has been minimized.

@ptomulik

ptomulik Apr 19, 2016

Contributor

Another thought -- to_s is usually meant to represent object in a string form for displaying purposes. Here it looks like it has a purpose of generating output file name. Is it intentional? Shouldn't these two things be separated?

This comment has been minimized.

@jordansissel

jordansissel Apr 19, 2016

Owner

Indeed! Good observation. In hindsight, maybe .to_s primarily for
formatting file names wasn't a great move.

On Monday, April 18, 2016, Paweł Tomulik notifications@github.com wrote:

In lib/fpm/package/deb.rb
#1093 (comment):

@@ -905,7 +905,7 @@ def mkdir_p(dir)
def to_s(format=nil)
# Default format if nil
# git_1.7.9.3-1_amd64.deb

  • return super("NAME_FULLVERSION_ARCH.TYPE") if format.nil?
  • return super("NAME_FULLVERSION_ARCH.EXTENSION") if format.nil?

Another thought -- to_s is usually meant to represent object in a string
form for displaying purposes. Here it looks like it has a purpose of
generating output file name. Is it intentional? Shouldn't these two things
be separated?


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
https://github.com/jordansissel/fpm/pull/1093/files/af92f4976143ff7a2932efcf79d69e4de35ab8a9#r60172211

@ptomulik

This comment has been minimized.

Contributor

ptomulik commented Apr 18, 2016

So, merge, maybe? :)

@@ -562,8 +562,8 @@ def epoch
def to_s(format=nil)
if format.nil?
return super("NAME-VERSION-ITERATION.DIST.ARCH.TYPE").gsub('DIST', attributes[:rpm_dist]) if attributes[:rpm_dist]
return super("NAME-VERSION-ITERATION.ARCH.TYPE")
return super("NAME-VERSION-ITERATION.DIST.ARCH.EXTENSION").gsub('DIST', attributes[:rpm_dist]) if attributes[:rpm_dist]

This comment has been minimized.

@ptomulik

ptomulik Apr 19, 2016

Contributor

This is something that looks strange to me. Why we need to perform substitutions on a fixed (ad-hoc) string? Merely, it's same as

super("NAME-VERSION-ITERATION.#{attributes[:rpm_dist]}.ARCH.EXTENSION") if attributes[:rpm_dist]

except the current substitution is made on a string already processed by super (which doesn't really matter ATM, as super does not introduce any "DIST" substrings). I'd suggest to delegate this gsub to FPM::Package#to_s for readability.

This comment has been minimized.

@ptomulik

ptomulik Apr 19, 2016

Contributor

Oh, no rpm_dist is RPM specific. Can't be delegated to FPM::Package.

This comment has been minimized.

@ptomulik

ptomulik Apr 19, 2016

Contributor

Anyway, if format argument is provided and is not nil and if it already contains "DIST" substring, the "DIST" substring will not be substituted.

@ptomulik ptomulik force-pushed the ptomulik:fix/freebsd-to_s branch from 765957d to 39dba94 Apr 19, 2016

@jordansissel

This comment has been minimized.

Owner

jordansissel commented Apr 19, 2016

LGTM

@jordansissel jordansissel merged commit 5c31fdf into jordansissel:master Apr 19, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ptomulik

This comment has been minimized.

Contributor

ptomulik commented Apr 19, 2016

Thanks.

jordansissel added a commit that referenced this pull request Jun 20, 2016

Merge pull request #1093 from ptomulik/fix/freebsd-to_s
fixes for FPM::Package#to_s and relatives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment