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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve version related functions #215

Merged
merged 1 commit into from
Feb 10, 2016

Conversation

matthewrmshin
Copy link
Member

fcm make: new required-version declaration.
fcm version: now displays FCM_HOME location, consistent with rose version.

Close #113.

@matthewrmshin matthewrmshin self-assigned this Nov 27, 2015
@matthewrmshin matthewrmshin added this to the next-release milestone Nov 27, 2015
fcm make: new `required-version` declaration.
fcm version: now displays FCM_HOME location, consistent with `rose version`.
@matthewrmshin
Copy link
Member Author

@benfitzpatrick @scwhitehouse please review.

@matthewrmshin matthewrmshin modified the milestones: next-release, 2015.12.0 Dec 9, 2015
@benfitzpatrick
Copy link
Contributor

Looks fine to me, passes the test battery.

my ($min_version, $max_version) = shellwords($entry->get_value());
if ( $min_version gt $version
|| defined($max_version) && $version gt $max_version
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand, is this doing a text comparison on the strings (rather than numeric, as 2015.12.0 isn't a number)? And if one value is set as require-version it's taken as a minimum, and if there are two the first is a minimum and the second is a maximum?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds about right. (Obviously, the numeric comparison will not work if we issue more than 10 formal releases in a single month, but I don't think we'll ever get to that for FCM.)

@scwhitehouse
Copy link
Contributor

In which case this looks sensible to me

scwhitehouse added a commit that referenced this pull request Feb 10, 2016
@scwhitehouse scwhitehouse merged commit 0d99bd2 into metomi:master Feb 10, 2016
@matthewrmshin matthewrmshin deleted the fcm-make-require-version branch February 10, 2016 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants