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

Expose Version#built_at on the versions index page #3115

Conversation

nvasilevski
Copy link
Contributor

Hey folks! Me and @Schwad would like to propose a change to the Version index page
We would like to use a bit more accurate date especially for gems that were imported into rubygems on July 25, 2009
Currently all gems use version.created_at (interestingly it was switched from version.built_at) - #1504
but for older versions of gems like nokogiri created_at has not a lot of meaning, for example:
https://rubygems.org/gems/nokogiri/versions

Currently on production for nokogiri image

So we went with a little verbose but backwards-compatible approach that falls back to built_at attribute for gems that we consider "imported", or, literally created on July 25th, 2009
Naming was hard so feel free to suggest your options, but we added a new method authored_at that performs this fallback by checking whether the version was seeded? which just compares created_at date to be equal July 25th, 2009 (time part is stripped)

Which results in a nicer and more insightful history of older nokogiri versions:

After using built_at for pre-rubygems gems image

Side-to-side comparison

image

@Schwad
Copy link
Contributor

Schwad commented Jun 27, 2022

This is great Nikita, thanks for this! I think it's really clear what we were working on and solving here.

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

+1, this is great work folks 🚀

One thing that I noticed in the PR that was linked about switching from built_at to created_at is that built_at is an author defined field in the gemspec that doesn't abide by any validations. For instance, I found that from gems with a created_at on July 25, 2009, there's a handful of gems with a built_at in the future.

To keep the information as accurate as possible, perhaps for versions before July 25, 2009, we can have the built_at date but have an asterisk beside those dates. We can specify that these versions were the specified built at date but was pushed to rubygems.org on July 25, 2009.

@nvasilevski
Copy link
Contributor Author

we can have the built_at date but have an asterisk beside those dates. We can specify that these versions were the specified built at date but was pushed to rubygems.org on July 25, 2009.

That makes total sense! I'll make a change
I forgot to mention this, but I definitely admit that using nokogiri as an example is a bit cheaty because I was confident it has proper built_at dates but I definitely was expecting some gems to have inconsistent built_at value 🙃

@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch from 48fabd3 to 83e5b74 Compare July 5, 2022 00:44
@nvasilevski
Copy link
Contributor Author

Update:
Added an asterisk for versions that were imported. Please feel free to suggest the most appropriate wording as I feel like I keep mixing seeded and imported as seeded sounds suitable from an application point of view but from a user point of view imported may have better clarity.

image

@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch from 83e5b74 to df23d2a Compare July 5, 2022 00:56
@nvasilevski
Copy link
Contributor Author

For instance, I found that from gems with a created_at on July 25, 2009, there's a handful of gems with a built_at in the future.

Also added an additional check to fall back to created_at for gems there were imported on July 25th, but authors specified date in the future. This should keep the history consistent.

@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch from df23d2a to 601354a Compare July 5, 2022 01:20
@@ -10,4 +10,9 @@
<%= render @versions %>
</ul>
</div>
<% if @versions.any?(&:rely_on_built_at?) %>
Copy link
Contributor

@Schwad Schwad Jul 5, 2022

Choose a reason for hiding this comment

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

This makes sense that we only show it if there are old gem versions

Copy link
Member

Choose a reason for hiding this comment

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

We should update line 7 since: nice_date_for .. created_at to use authored_at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, going to update it 👍
Thank you!

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

Thanks @nvasilevski, I just have a couple of questions/suggestions, but otherwise it LGTM!

@qrush
Copy link
Member

qrush commented Jul 5, 2022

GitHub ate my email on this, apologies if this goes through twice.

Some background/history on this attribute in case we need is on #1353, also @sonalkr132 did a lot of work in this space too and may want to review this.

I had a 13 year old code flashback as I remember writing the migration to add built_at (and deal with its confusion with created_at) but having trouble recalling the discussion around it. This was before PRs on GitHub and possibly deep in the IRC logs on freenode if those exist somewhere…

@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch from e4d3478 to 3546074 Compare July 5, 2022 16:51
@nvasilevski
Copy link
Contributor Author

Addressed suggestions & rebased on the latest main to remove unrelated translations moves

@Schwad
Copy link
Contributor

Schwad commented Jul 6, 2022

I had a 13 year old code flashback as I remember writing the migration to add built_at (and deal with its confusion with created_at) but having trouble recalling the discussion around it. This was before PRs on GitHub and possibly deep in the IRC logs on freenode if those exist somewhere…

My impression is that since created_at was at one point "polluted", or possibly full of data that may have been editable by the author.

So on July 25, 2009 that PR made a copy of that data on built_at and established canonical created_at dates beginning from that date forward.

Although this is quite old, I find this PR incredibly helpful for those of us (like me) who spend a lot of time in "old" ruby and would benefit massively from a possibly more accurate representation of version dates before then.

For example with @flavorjones Bundler::AsOf which lets us bundle a gemfile and resolve dependencies from, say, 2008.

Thanks for the work and feedback everyone

@jenshenny jenshenny requested a review from sonalkr132 July 13, 2022 15:26
@sonalkr132
Copy link
Member

This change seems fine to me. Please make sure that we didn't bulk import versions on any other day as well.
Also, note that this mostly an aesthetic change. No API response or functions which rely on dates will be changed.

@Schwad
Copy link
Contributor

Schwad commented Jul 18, 2022

Also, note that this mostly an aesthetic change. No API response or functions which rely on dates will be changed.

I think this makes sense. As the formal api already returns built_at, so for an issue I'm currently working (and others in this position) we can manually revert to built_at for gems imported on the import date.

@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch 2 times, most recently from a20e441 to d8f6e25 Compare July 18, 2022 17:15
@nvasilevski
Copy link
Contributor Author

Update:
We went with a tooltip to make sure the notice is going to be rendered on both Versions#index and RubyGems#show page
Preview:
image

klass = ["gem__version__date"]
text = version_authored_date(version)
if version.rely_on_built_at?
klass << "tooltip__text"
Copy link
Member

@sonalkr132 sonalkr132 Jul 19, 2022

Choose a reason for hiding this comment

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

You should be able to add a modifier class to preserve the version number color. As per your screenshot, ones will tooltip are now red.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable! I wasn't sure if we want to preserve the color or highlight it to be aligned with other tooltips

I defined .gem__version__date.tooltip__text style explicitly to preserve the color from gem__version__date
Let me know if that is what you meant

image

Copy link
Member

Choose a reason for hiding this comment

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

This looks much better. Thank you.

Co-authored-by: Nick Schwaderer <nick.schwaderer@shopify.com>
@nvasilevski nvasilevski force-pushed the expose-original-built-at-date-on-versions-index branch from d8f6e25 to 3c67f90 Compare July 25, 2022 14:03
@sonalkr132 sonalkr132 merged commit 876c33d into rubygems:master Jul 28, 2022
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging August 3, 2022 17:40 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production August 3, 2022 17:45 Inactive
@sonalkr132
Copy link
Member

sonalkr132 commented Aug 3, 2022

This was deployed to prod. One improvement we can make is update the tooltip icon. [?] on some version and not on others, feels wonky. A better icon could be a superscript * next to version number, which would better covey that some version numbers are special/different.

@nvasilevski nvasilevski deleted the expose-original-built-at-date-on-versions-index branch August 3, 2022 19:38
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.

5 participants