Skip to content

Conversation

@rpetrusha
Copy link

Revisions for Reiwa era

@tarekgh

@rpetrusha rpetrusha added this to the April 2019 milestone Apr 1, 2019
@rpetrusha rpetrusha self-assigned this Apr 1, 2019
@rpetrusha rpetrusha requested a review from BillWagner as a code owner April 1, 2019 20:14
@tarekgh
Copy link
Member

tarekgh commented Apr 1, 2019

I would avoid showing the exact output of this code because it will need to change again in the future, and also the code depends on the date pattern "d" which can be easily changed on the system and produce different output.


Refers to: snippets/standard/datetime/calendars/current-era/cs/Program.cs:22 in c61e2ce. [](commit_id = c61e2ce, deletion_comment = False)

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Overall, this LGTM @rpetrusha

Once you and @tarekgh have worked through changes, you can :shipit:.

@tarekgh
Copy link
Member

tarekgh commented Apr 2, 2019

I would say, we need to change all doc sample here to not mention the exact output but instead explain what to expect the output will look like. the reasons are:

  • Showing the output will depends on the machine settings (e.g. the machine has the era updates and what date format settings).
  • We'll have to revisit all these samples again every time we have a new era.
  • The output possibly can be different on Linux machines depending on the ICU version we use there.

@rpetrusha
Copy link
Author

@tarekgh, I've removed output except for the leap year/day examples, since leap years/days have historically caused confusion. In all other cases, I've modified the text to describe the output. I'll merge this, but please feel free to review these changes (as well as the changes in the related PRs) and let me know if you'd like anything further to be changed.

@rpetrusha rpetrusha merged commit 865e607 into dotnet:master Apr 3, 2019
@rpetrusha rpetrusha deleted the reiwa-era branch April 3, 2019 20:30
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.

3 participants