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

Add new :append_datetime param and fix dates to UTC. #53

Merged
merged 5 commits into from
Mar 13, 2021

Conversation

isadon
Copy link
Contributor

@isadon isadon commented Dec 17, 2020

This PR adds a new stamp_changelog action argument: stamp_date_time. Specifying true stamps the changelog in the format of standard iso8601 date time "2020-12-17T22:25:13Z". By default it is false and when true it overrides stamp_date. The date chosen, whether by specifying stamp_date: true or stamp_date_time: true are placed in the env variable FL_UPDATE_DATE_VAL, FL_UPDATE_DATE_TIME_VAL respectively.

It also fixes the error being given by fastlane that the stamp_date arg was not a string and makes all the dates used for the stamp_dates be in UTC time.

I figure this PR may require some additional modifications depending on the needs of the project but I figured it could be built upon 😄.

@fastlane-changelog-bot
Copy link

1 Error
🚫 Please include a CHANGELOG entry to credit yourself!
You can find it at CHANGELOG.md.

Here's an example of your CHANGELOG entry under [Unreleased] section:

### Fixed:
- Add new :append_datetime param and fix dates to UTC.

Generated by 🚫 Danger

@pajapro
Copy link
Owner

pajapro commented Dec 20, 2020

Hi @donileo ,

thanks a lot for the idea how to improve the plugin and actual contribution ❤️. I understand your need from issue #52

I'm looking to include the time along with the date in the form of 2020-12-08T21:51Z

the approach (add additional stamp_date_time param in update_changelog.rb and stamp_changelog.rb ) you have chosen in this PR to satisfy your need is OK, but I see 2 potential problems long-term:

  1. testability & complexity : adding an additional param increases the code complexity and what has to be tested
  2. confusion for users : it may be confusing at glance to figure out why this plugin has 2 params for date/time instead of just one

Suggestion for improvement:

Instead of adding a new param for date & time format, I suggest to enhance the existing stamp_date and let the user specify (override) the (default) format of date/time, here is how:

  1. rename stamp_date param to something like.....e.g. stamp_datetime_format?
  2. make stamp_date's default_value %Y-%m-%d - so that we stay backwards compatible.
  3. make stamp_date's is_string true
  4. Once we have the above interface in place, any user (such as yourself 😉) can pass in stamp_datetime_format: %FT%T%:z, which will result in 2020-12-08T21:51Z (see doc)
  5. In the update_changelog.rb you won't need this if/else and instead leverage the strftime Ruby method to create date & time format specified via string 👍
  6. Please remember to add unit tests to cover the new functionality

Copy link
Owner

@pajapro pajapro left a comment

Choose a reason for hiding this comment

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

See my comment about overall approach to satisfy the need to change format of the stamped date & time

@isadon
Copy link
Contributor Author

isadon commented Mar 6, 2021

Wow I forgot about this 😄 lol. That's what happens when you get caught up with work, implement your own solution that works for your own cases and then forget about requested changes such as the above lol. Let's see if I can do this today 😄, no promises..

@isadon
Copy link
Contributor Author

isadon commented Mar 6, 2021

Ok doing this now. Good suggestions. Some observations:

  1. On the make stamp_date's default_value %Y-%m-%d - so that we stay backwards compatible. there is an issue with "staying compatible". Staying compatible means using keep a changelog's bad time format of not specifying whether the date being stamped is in local time or UTC/Zulu time. If we assume that by this time: %Y-%m-%d they mean local time their standard needs to specify the timezone that time is in but they don't do that. Given all of that I'm making the default assumption that the time being stamped is UTC time and as such append Z to the end of the date formats to let the user know this so it becomes this: %Y-%m-%dZ. That is why you see the UTC references in my commit 😄. Please see this for reference: Specifying time in release date. olivierlacan/keep-a-changelog#375

  2. The changes as you've requested them above mean that stamp_datetime_format becomes a string from what it used to be: stamp_date (a boolean). This now means that the user has no way of doing the old functionality which is I don't want any date at all. Fastlane's plugin default_value functionality means that even if the user passes in nil for stamp_datetime_format that the default_value will be applied. Given this it seems this change would be a breaking change a users now will always have a date or datetime stamped to their sections where as before they could've state stamp_date: false and have none applied. Maybe you are implying we keep stamp_date as a boolean as well?

Update: For item 2 I went ahead and added the ability not to stamp the date via the should_stamp_date/should_append_date param. It will still be a breaking change due to the param name change from stamp_date to should_stamp_date but much better than lacking the ability to not stamp dates at all.

Date time format is in Ruby's strftime format and is not required. If
not passed no date is stamped on the changelog section.
Copy link
Owner

@pajapro pajapro left a comment

Choose a reason for hiding this comment

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

Nice, approve and going to release it 🎉 thank you for contribution once again @donileo

@@ -80,12 +80,35 @@
expect(modifiedSectionLine).to eq("## [12.34.56]\n")
end

it 'updates [Unreleased] section identifier with appending date' do
# Update [Unreleased] section identifier with new one
it 'updates [Unreleased] section identifier appending a date via the default datetime format' do
Copy link
Owner

Choose a reason for hiding this comment

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

Perfect

@pajapro pajapro merged commit 64168bd into pajapro:master Mar 13, 2021
@isadon
Copy link
Contributor Author

isadon commented Mar 13, 2021

@pajapro Thanks 👍, no problem.

I think another change (not sure if its considered breaking) that needs to be mentioned in the release is that the actual stamped date is now in UTC format - not local since previously it was using Time.now which returned local time. I feel that is important enough to possibly even mention in other places like in the stamp_datetime_format for which I'll make a future PR. Please let me know your thoughts.

@pajapro
Copy link
Owner

pajapro commented Mar 13, 2021

Good point, for now I at least updated release notes.

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.

stamp_changelog: stamp_date does not support a user passed in date time string
3 participants