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

Started working on having the ability to have a different license for each version of a crate. #787

Merged
merged 32 commits into from
Jun 22, 2017

Conversation

baileyn
Copy link
Contributor

@baileyn baileyn commented Jun 19, 2017

This is some work towards fixing #736

baileyn added 30 commits June 11, 2017 13:55
…he license and validate it. Additionally, Made NewCrate.crate_or_update take an immutable self instead of mutable because it was no longer necessary.
…he license and validate it. Additionally, Made NewCrate.crate_or_update take an immutable self instead of mutable because it was no longer necessary.
…he license and validate it. Additionally, Made NewCrate.crate_or_update take an immutable self instead of mutable because it was no longer necessary.
…he license and validate it. Additionally, Made NewCrate.crate_or_update take an immutable self instead of mutable because it was no longer necessary.
@carols10cents
Copy link
Member

Looks like rustfmt is still sad :( I'm not sure why it's different... I'm going to try running it on here.

@carols10cents
Copy link
Member

This is going well!! I love the VersionBuilder in the tests ❤️

I tried out the code locally, and publishing versions locally was doing what I would expect as far as continuing to update the crate license, continuing to reject invalid licenses, and starting to keep track of the licenses in the versions table 💯

One small issue I found when running update-licenses however: the license column on the crate table can be null (that is, we haven't put a NOT NULL restriction on it in the database), so of course that means there are some crates with a null license in the production database (36 of them. and somehow one in my local database??) 😱😱😱

I'm going to file a separate issue for that, because I think we should be able to make it not null, but I'm not sure how it's happening, if it's something that's been fixed or not, so I think in this PR we should support a null license.

Currently, though, when update-licenses sees a null value, I get this error:

thread 'main' panicked at 'error retrieving column "license": Conversion(WasNull)', /Users/carolnichols/.cargo/registry/src/github.com-1ecc6299db9ec823/postgres-0.14.0/src/rows.rs:206
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic
   5: std::panicking::begin_panic_fmt
   6: postgres::rows::Row::get
   7: update_licenses::transfer
   8: update_licenses::main
   9: __rust_maybe_catch_panic
  10: std::rt::lang_start
  11: main

I think on line 30 of update-licenses.rs we can just change the type annotation from String to Option<String> and it should work? Do you mind trying that out and letting me know?

Since I'm not sure how crates.io is creating crates with null licenses, you can change a crate in your database to have a null license in psql by running:

UPDATE crates SET license = NULL where crates.name='whatever';

Thank you!!! Let me know if you have questions!!!

assert!(num_updated > 0);
let license: Option<String> = row.get("license");

if let Some(license) = license {
Copy link
Member

Choose a reason for hiding this comment

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

nice use of if let!

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 and the script works great with null licenses now too! ❤️

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