Fixes #28766 - add version macro#7361
Conversation
|
Issues: #28766 |
| end | ||
|
|
||
| def version(version) | ||
| Gem::Version.new(version.to_s) |
There was a problem hiding this comment.
potentially we can rescue from ArgumentError which is raised on malformed string and return Gem::Version.new(0) (nil is discouraged), though that can be misleading in comparisons, e.g. it will always be smaller than any other version
perhaps raising some better rendering error would be best, thoughts?
There was a problem hiding this comment.
How difficult would using a proxy object (That we control) be?
There was a problem hiding this comment.
Not something I'd do in next month :-) do you have some more complicated use of it in mind? Or you don't want to rely on Gem not chaning it's API?
There was a problem hiding this comment.
The latter. I fear we‘re introducing technical depth here.
There was a problem hiding this comment.
You could do what Puppet has done: a versioncmp function that is basically Gem::Version.new(first) <=> Gem::Version.new(second). That API might be easier to guarantee in the future.
For reference, I know that RPM and Debian version comparisons are slightly more complex and different than semver. Probably good to at least state what it's based on.
There was a problem hiding this comment.
What Ewoud says comes to my mind too, the most clean way. Hides the implementation completely.
|
I don't think failures are related |
|
well given it's EOD anyway, let's rerrun to be sure :-) [test foreman] [test katello] katello didn't see http_proxies table |
|
katello still fails the same, but that's unrelated, all the rest is green |
lzap
left a comment
lzap
left a comment
There was a problem hiding this comment.
Other than that, I am good. Consider a different name since we might want to use version to return foreman version, probably version_compare or something like that. But I don't insist.
| @@ -0,0 +1,3 @@ | |||
| class Gem::Version::Jail < Safemode::Jail | |||
| allow :<, :>, :<=, :>=, :==, :eql?, :prerelease?, :release, :version, :to_s, :bump, :canonical_segments | |||
| end | |||
|
I updated based on suggestions, only one new macro for version comparison is added and it's name reflects better what it does. We lost few useful macros this way (:prerelease?, :release, :version,, :bump, :canonical_segments) however I hope this can now get in. @lzap, community template report needs to be updated afterwards, it relied on |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
How does it deal with nil? Currently I'd say that's undefined (no tests) and perhaps not that relevant, but perhaps you want to make a statement about that.
| :number_with_precision, | ||
| :number_to_human_size | ||
| :number_to_human_size, | ||
| :gem_version_compare |
There was a problem hiding this comment.
I thought we had trailing comma's but looks like we don't here. @mmoll is that a missing rubocop rule?
There was a problem hiding this comment.
Yes, that should be Style/TrailingCommaInArrayLiteral... I'd need to have a deeper look to find out, if that's a Rubocop bug.
6 participants
No description provided.