Skip to content

Show whether a crate version has been yanked.#18

Open
mattdeboard wants to merge 3 commits intoonelson:mainfrom
mattdeboard:show-yanked
Open

Show whether a crate version has been yanked.#18
mattdeboard wants to merge 3 commits intoonelson:mainfrom
mattdeboard:show-yanked

Conversation

@mattdeboard
Copy link
Contributor

@mattdeboard mattdeboard commented Jan 2, 2021

The fact the crate data is harvested from the git reflog influences how I approached this. We're basically iterating over log lines, but since a single package & version pair can have multiple reflog lines, I needed a way to keep track of what package/version pairs I've already seen, and whether or not they have been yanked.

HashMap<(String, String), bool> seemed like a sensible choice for data structure here for fast lookups & duplicate elimination. The get_publishes function is only responsible for generating the tell-tale data structure. The logic to "massage" that data into display format is handled in frontend::landing.

@mattdeboard
Copy link
Contributor Author

mattdeboard commented Jan 2, 2021

I need to update this to allow for yank --undo overwriting the bookkeeping

edit: And actually I'm not sure if the "parse reflog" approach will still work. The reflog is basically time series data given in reverse. So maybe just parsing the JSON for each crate and all is the better way. I'll work on that.

@onelson
Copy link
Owner

onelson commented Jan 2, 2021

I actually never figured out how to unyank via cargo. TIL!

@onelson
Copy link
Owner

onelson commented Jan 2, 2021

So maybe just parsing the JSON for each crate and all is the better way. I'll work on that.

Parsing the index files makes the most sense in terms of getting the package info, correct. The thing that's missing is when things happened.

@mattdeboard
Copy link
Contributor Author

If we create our own command to invoke a custom command line, e.g. git reflog —date=iso, and read the reflog lines from there, we can get the dates in the log message, even tell git exactly how we want the date formatted so we dont have to do that in rust code.

@onelson
Copy link
Owner

onelson commented Jan 2, 2021

A long term goal of mine will be to remove the need to shell out, so if we can avoid it for this, it'd be better. I'd be happy to see a dep on chrono added to assist with date parsing and formatting.

@onelson
Copy link
Owner

onelson commented Jan 2, 2021

@mattdeboard I took a look at the diff and I think things are looking pretty good. I had a couple of thoughts.

Since the landing page currently acts a bit like an activity feed, this diff could be simplified along the lines of:

  • updating the return type of get_publishes() to Result<Vec<(String, String, String)>>
  • where (String, String, String) is "verb", crate name, and version
  • optionally add a 4th element with some representation of the date for the reflog entry
  • optionally use a struct with named fields instead of a tuple

With that, we could go back to using a limit and skip the HashMaps.

The get_publishes() method could likely be renamed to account for it giving more than just the publish events. Likewise, updating the templates to say like "recent activity" and "all activity" or something.

How does that sound?

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.

2 participants