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.

Sign up for GitHub

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

Jump to bottom

Licenses #803

Merged
merged 4 commits into from
Jul 22, 2017
Merged

Licenses #803

merged 4 commits into from
Jul 22, 2017

Conversation

baileyn
Copy link
Contributor

@baileyn baileyn commented Jun 25, 2017 •
edited
Loading

Fixed the front-end to display licenses based on versions rather than crates. I also added a test to make sure the licenses were displayed correct and changed based on version. This should finish up #736. The only thing that's left to do now is remove the old code there for crate-based versions.

@carols10cents
Copy link
Member

Hi @baileyn sorry I've taken so long to review this again!! Taking a look now :) ❤️

@carols10cents
Copy link
Member

This code is looking great!! I just have a few things:

  • I rebased this branch on master and force pushed, so please fetch and rebase to get my changes-- there are some folks helping out making the ember code and tests better, and they're starting to move the mirage fixtures to mirage factories, so since we changed that out from under you, I went ahead and updated your code here :) (plus this was an opportunity to get to play with the fixtures :)) Please let me know if you have any questions about factories vs. fixtures!

  • Since we're removing the license key from crate JSON in the fixtures/factories, and since the fixtures/factories represent the JSON that the backend returns, I think this PR would be a great time to remove the license key from EncodableCrate. This will change everywhere we create JSON for crates, but the license will still be in the database (since that's controlled by the Crate struct), so if we need to roll this back for some reason, we won't have lost any data. You might get compiler errors or Rust test errors when removing the license field from EncodableCrate-- do you mind removing the field and fixing whatever errors come up? Does this make sense?

  • Since you've done a few PRs now, I think you're ready for one of my very favorite git commands that I use all the time-- it's called "interactive rebase" and it lets you change your commits around after you've made them :) It looks like your commit message for your first commit, "Maybe now I fixed the formatting." was kind of a development note, which is totally fine and I do that all the time too (you should see how many commits I have locally that just say "poop" haha). However, if someone else was looking back on the history of commits and wanted to know why the changes in the commit were being made, the message here doesn't really describe the change-- that the license displayed on crate and version pages is now coming from the version JSON rather than the crate JSON, and that this is part of fixing Show licenses of crates on a per version basis #736. One of the things interactive rebasing does is lets you change your commit message easily!

The way I usually do interactive rebasing is git rebase -i upstream/master (where upstream is your remote that points to github.com/rust-lang/crates.io), which will rebase your branch on the latest master and will put you in vim with all of your commits that are not yet on master listed at the top. There are instructions in comments at the bottom of what you can do here-- the one you'll want here is r for reword, so change the pick next to your first commit to say r then :wq out of vim. Next you'll be put into vim editing that commit's message! Make it a bit more descriptive, then :wq, then the rebase will finish. You'll have to force push to github again, and then you should be all set.

There's some more documentation about interactive rebasing here: https://help.github.com/articles/about-git-rebase/ Please let me know either here or on IRC if you have any questions!! ❤️

@baileyn
Copy link
Contributor Author

baileyn commented Jul 16, 2017

Sorry it's taken me so long to reply, @carols10cents , I've been helping my parents move over the past couple of weeks so I haven't been near my development box. I'm heading back home today so I'll be sure to take a look at resolving all of the issues you've mentioned.

@carols10cents
Copy link
Member

No apologies necessary :) ❤️

carols10cents
carols10cents approved these changes Jul 22, 2017
View reviewed changes
Copy link
Member

@carols10cents carols10cents left a comment

Choose a reason for hiding this comment

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

Looks great works great!! Thank you!!! ❤️ 💟

@carols10cents carols10cents merged commit c1c7efb into rust-lang:master Jul 22, 2017
@carols10cents
Copy link
Member

And this is deployed! Check it out-- I've got a crate that has v0.1.2 licensed MIT https://crates.io/crates/carol-test/0.1.2 and v0.1.3 licensed MIT/Apache-2.0 https://crates.io/crates/carol-test !! Wooooo!!!

@baileyn baileyn deleted the licenses branch July 23, 2017 16:34
@baileyn
Copy link
Contributor Author

baileyn commented Jul 23, 2017

Wooooo! It's so exciting to see something I made actually work on a production server 😁

@iKevinY iKevinY mentioned this pull request Aug 27, 2017
Merged
paulmelnikow pushed a commit to badges/shields that referenced this pull request Sep 24, 2017
@iKevinY @paulmelnikow
Fix [crates] license badge (#1062)
c1a8166 
License data was moved to crate versions in rust-lang/crates.io#803.
@rye rye mentioned this pull request Aug 28, 2019
Merged
bors added a commit that referenced this pull request Sep 5, 2019
@bors
Auto merge of #1815 - rye:remove-crates-license-column, r=sgrif
66c60dc 
Remove `license` column on `crates` table

Seemingly the last step in the checklist on #736, this PR removes the `license` column on the `crates` table, and all references thereto. (In fact, I think #736 can be closed once this PR is merged.)

As of #803, the front-end no longer really used this field, and the front-end was also changed to render the license of the _current_ version, instead of the value coming from this now-obsolete `license` column on the crate. Thus, this column likely contains stale data and, to my knowledge, isn't changed at all when a new version is uploaded.

I wasn't sure what requirements are enforced for backwards-compatibility, or whether my changes here would actually affect behavior of e.g. if there are old versions of `cargo` that will upload with these values. I am happy for these changes to be scrutinized until this is verified to be safe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carols10cents carols10cents carols10cents approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@baileyn @carols10cents @Turbo87