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

Incorporate release date with ensmallen version #226

Merged
merged 8 commits into from Oct 30, 2020
Merged

Incorporate release date with ensmallen version #226

merged 8 commits into from Oct 30, 2020

Conversation

coatless
Copy link
Contributor

@coatless coatless commented Sep 6, 2020

From #222, we discussed adding date information to the header file for when ensmallen was released/updated. This PR includes the version update within ens_version.hpp and updates the release script to automatically set these fields.

@conradsnicta
Copy link
Contributor

conradsnicta commented Sep 7, 2020

What's the use case for the date? Having a date appears redundant. The version number already contains all the necessary information to determine the functionality (ie. API state) and bug fixes present within a given release. I'm not convinced by the reasoning provided in #222.

A further problem with adding the date is that it adds more things to keep track of (ie. adds to the maintenance burden and risk of bugs), while providing no compelling value.

If somebody really needs to know the date of a release, they can always look at the tags or perhaps the date stamps within a release.

@coatless
Copy link
Contributor Author

coatless commented Sep 7, 2020

@conradsnicta thanks for the question. The reason for the date being embedded in ens_version.hpp was to simplify extraction. Prior to that, there was a hiccup during release when the tagged version lacked both a version number/date, which was problematic for generating an automatic upstream PR refresh with RcppEnsmallen's upstream-refresh GitHub Action.

Now onto the other point, I do agree except for the fact that ensmallen has a real nice release utility @rcurtin worked on: scripts/ensmallen-release.sh. The version and date information is automatically set and a release candidate PR is created.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Given the automatic release script, I agree that there's not much maintenance burden for keeping this up-to-date. Another way to get the date would be something like this:

grep '^######' HISTORY.md  | grep -v '????' | head -1 | sed 's/^###### //'

and then you have the release date in YYYY-MM-DD form.

Anyway, I'm fairly ambivalent about what gets chosen here---if it really does make your life easier to add it here, great, I don't have a problem with merging it, but if you have another way to get the same information without adding extra complexity (even though we can hide that in the automatic release scripts), I'd probably lean that way. Let me know what you think. 👍 (And sorry for the slow response!)


std::stringstream ss;
ss << version::major << '.' << version::minor << '.' << version::patch
<< " (" << nickname << ')';
<< " (" << nickname << ")\n"
<< "Released on " << year << '-' << month << '-' << day;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like someone would expect ens::version::as_string() to return something very simple without a newline; if we do go with this idea, maybe we should make an additional function date() that returns year << '-' << month << '-' << day?

@rcurtin
Copy link
Member

rcurtin commented Oct 27, 2020

Hey @coatless, just trying to see this one through---what do you think? Either approach is fine with me. 👍

@coatless
Copy link
Contributor Author

@rcurtin sorry for missing this. I've been swamped the last few weeks. I'll aim to add a second function.

@coatless
Copy link
Contributor Author

@rcurtin split the function. Looks like everything is passing.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Awesome, looks good to me. Thanks @coatless! 👍 And no worries about being swamped; I know how busy things can get sometimes...

include/ensmallen_bits/ens_version.hpp Outdated Show resolved Hide resolved
Co-authored-by: Ryan Curtin <ryan@ratml.org>
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

include/ensmallen_bits/ens_version.hpp Outdated Show resolved Hide resolved
Comment on lines 51 to 56
const char* year = ENS_VERSION_YEAR;
const char* month = ENS_VERSION_MONTH;
const char* day = ENS_VERSION_DAY;

std::stringstream ss;
ss << year << '-' << month << '-' << day;
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to not pass the date into the stream directly:

ss << ENS_VERSION_YEAR << '-' << ENS_VERSION_MONTH << '-' << ENS_VERSION_DAY;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoq No reason not to. I opted to mirror the styling of nickname if memory serves correctly.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks for putting this together.

@coatless
Copy link
Contributor Author

@rcurtin / @zoq if possible -- when this is merged in -- could you use squash and, then, merge? No need for keeping all of these commits in the repo history.

@rcurtin rcurtin merged commit d53815d into mlpack:master Oct 30, 2020
@rcurtin
Copy link
Member

rcurtin commented Oct 30, 2020

Sure! Thanks again for adding this. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants