feat(publish): add --access flag to publish command #299

ashleygwilliams · 2018-09-11T21:38:56Z

fixes #297

ashleygwilliams · 2018-09-11T21:45:41Z

src/command/publish.rs

+        match s {
+      "public" => Ok(Access::Public),
+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),


doing something a little unconventional here but i seriously cannot believe that it's "restricted" and not "private" so i kiiiiiinda want to have this in here, but if ya'll object i'm cool with removing it

So the npm wording is "restricted" but this is creating an alias to that named "private"? I'd prefer just matching what npm does.

The indentation here is super funky too

I like the alias personally. wow cargo fmt what is you doing.

i talked with nick and am gonna file an issue, it's a bug in cargo fmt, likely from the long string literal

ashleygwilliams · 2018-09-11T21:46:05Z

src/command/publish.rs

+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),
+      _ => Err(Error::Unsupported { message: format!("{} is not a supported access level. See https://docs.npmjs.com/cli/access for more information on npm package access levels.", s)}),
+    }


also really not sure what's up with the formating here, i ran cargo fmt but this is nasty

Maybe if you manually add some \ followed by a newline in the middle of the format string?

ashleygwilliams · 2018-09-11T22:03:06Z

i actually just realized that if you pass --access="" npm gets mad. i was hoping it was just do it's default. do we want to change how the access level defaults? if yes, i can make a small change, if no i can make a bigger one. i'm thinking probably no, so this will require a small amount of work before it can be merged.

fitzgen · 2018-09-11T22:30:20Z

do we want to change how the access level defaults? if yes, i can make a small change, if no i can make a bigger one. i'm thinking probably no, so this will require a small amount of work before it can be merged.

While I personally think that access should default to public, I don't think it is worth creating inconsistencies with npm over.

fitzgen

Nice! Will be great to have this fixed.

A couple tiny suggestions below, that I think would improve things a bit.

fitzgen · 2018-09-11T22:31:31Z

src/command/mod.rs

+        #[structopt(long = "access", short = "a")]
+        access: Option<Access>,
+
+        /// The access level for the package to be published


This comment should be on the field above, and the comment for the field above should be here :-p

d'oh. good catch.

This is still unaddressed ;)

fitzgen · 2018-09-11T22:33:15Z

src/command/publish.rs

+        match s {
+      "public" => Ok(Access::Public),
+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),


So the npm wording is "restricted" but this is creating an alias to that named "private"? I'd prefer just matching what npm does.

fitzgen · 2018-09-11T22:33:50Z

src/command/publish.rs

+        match s {
+      "public" => Ok(Access::Public),
+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),


The indentation here is super funky too

fitzgen · 2018-09-11T22:34:23Z

src/command/publish.rs

+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),
+      _ => Err(Error::Unsupported { message: format!("{} is not a supported access level. See https://docs.npmjs.com/cli/access for more information on npm package access levels.", s)}),
+    }


Maybe if you manually add some \ followed by a newline in the middle of the format string?

fitzgen · 2018-09-11T22:36:25Z

src/command/publish.rs

+            Restricted => "restricted",
+        },
+        None => "",
+    };


How about instead of making this stringly typed again, we implement Display for Access, implement Default for Access, and then right here do

access.unwrap_or_default()

I agree with the use of Display to not have to worry about strings and have it be centralized to one spot so that calls to to_string() will be consistent as well

ack. i just reread your message and realized i had missed the "again". anyways, yes, using Display is a good call.

the one thing to note is that we can't implement a default- this is because the default changes depending on whether or not the package is scoped, which we don't currently track. i need to think about whether i think it's a good idea to do so, but i had a plan for how to do the default that didn't involve tracking whether it was scoped so i may try that first.

fitzgen · 2018-09-11T22:37:37Z

src/npm.rs

@@ -18,10 +18,11 @@ pub fn npm_pack(path: &str) -> Result<(), Error> {
 }

 /// Run the `npm publish` command.
-pub fn npm_publish(path: &str) -> Result<(), Error> {
+pub fn npm_publish(path: &str, access: &str) -> Result<(), Error> {


And then this argument would be access: Access and we know that we can't ever pass a bad access here

mgattozzi

I pretty much agree with @fitzgen on most of this so if you make the changes he suggested (I personally want the alias) then you can consider this an approval from me

mgattozzi · 2018-09-12T00:01:51Z

src/command/publish.rs

+        match s {
+      "public" => Ok(Access::Public),
+      "restricted" => Ok(Access::Restricted),
+      "private" => Ok(Access::Restricted),


I like the alias personally. wow cargo fmt what is you doing.

mgattozzi · 2018-09-12T00:03:07Z

src/command/publish.rs

+            Restricted => "restricted",
+        },
+        None => "",
+    };


I agree with the use of Display to not have to worry about strings and have it be centralized to one spot so that calls to to_string() will be consistent as well

ashleygwilliams · 2018-09-12T16:35:23Z

failing re doc comments now- gonna fill those out but i think i have one more refactor to make this nicer. it's too bad that the npm access flag doesn't take a default, e.g. --access=default, but i have a plan, that also cleans up the duplication in src/npm.rs

fitzgen

Looks great! r=me with that comment below fixed :)

fitzgen · 2018-09-14T20:42:31Z

src/command/mod.rs

+        #[structopt(long = "access", short = "a")]
+        access: Option<Access>,
+
+        /// The access level for the package to be published


This is still unaddressed ;)

mgattozzi · 2018-09-14T21:32:21Z

This LGTM great job! Let's get it merged

ashleygwilliams added needs docs please add docs to this PR needs review labels Sep 11, 2018

ashleygwilliams requested review from fitzgen and mgattozzi September 11, 2018 21:38

ashleygwilliams force-pushed the access-flag branch from 0cdf845 to fb32af4 Compare September 11, 2018 21:43

ashleygwilliams commented Sep 11, 2018

View reviewed changes

ashleygwilliams added the blocked label Sep 11, 2018

ashleygwilliams added question Further information is requested work in progress do not merge! labels Sep 11, 2018

ashleygwilliams changed the title ~~feat(publish): add --access flag to publish command~~ [WIP] feat(publish): add --access flag to publish command Sep 11, 2018

fitzgen reviewed Sep 11, 2018

View reviewed changes

mgattozzi reviewed Sep 12, 2018

View reviewed changes

ashleygwilliams force-pushed the access-flag branch 2 times, most recently from 61080b7 to b57c947 Compare September 14, 2018 17:03

ashleygwilliams added needs rebase and removed needs docs please add docs to this PR work in progress do not merge! blocked question Further information is requested labels Sep 14, 2018

ashleygwilliams force-pushed the access-flag branch 2 times, most recently from 0da4fa1 to 3ccc638 Compare September 14, 2018 20:41

fitzgen approved these changes Sep 14, 2018

View reviewed changes

feat(publish): add --access flag to publish command

86a4284

ashleygwilliams force-pushed the access-flag branch from 3ccc638 to 86a4284 Compare September 14, 2018 20:50

ashleygwilliams removed needs rebase needs review labels Sep 14, 2018

ashleygwilliams changed the title ~~[WIP] feat(publish): add --access flag to publish command~~ feat(publish): add --access flag to publish command Sep 14, 2018

ashleygwilliams added this to the 0.5.0 milestone Sep 14, 2018

mgattozzi merged commit 6ad8b0f into master Sep 14, 2018

ashleygwilliams deleted the access-flag branch January 13, 2019 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(publish): add --access flag to publish command #299

feat(publish): add --access flag to publish command #299

ashleygwilliams commented Sep 11, 2018

ashleygwilliams Sep 11, 2018

fitzgen Sep 11, 2018

fitzgen Sep 11, 2018

mgattozzi Sep 12, 2018

ashleygwilliams Sep 12, 2018

ashleygwilliams Sep 11, 2018

fitzgen Sep 11, 2018

ashleygwilliams commented Sep 11, 2018

fitzgen commented Sep 11, 2018

fitzgen left a comment

fitzgen Sep 11, 2018

ashleygwilliams Sep 12, 2018

fitzgen Sep 14, 2018

fitzgen Sep 11, 2018

fitzgen Sep 11, 2018

fitzgen Sep 11, 2018

fitzgen Sep 11, 2018

mgattozzi Sep 12, 2018

ashleygwilliams Sep 12, 2018 •

edited

Loading

ashleygwilliams Sep 12, 2018

fitzgen Sep 11, 2018

mgattozzi left a comment

mgattozzi Sep 12, 2018

mgattozzi Sep 12, 2018

ashleygwilliams commented Sep 12, 2018

fitzgen left a comment

fitzgen Sep 14, 2018

mgattozzi commented Sep 14, 2018

feat(publish): add --access flag to publish command #299

feat(publish): add --access flag to publish command #299

Conversation

ashleygwilliams commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams commented Sep 11, 2018

fitzgen commented Sep 11, 2018

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams commented Sep 12, 2018

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi commented Sep 14, 2018

ashleygwilliams Sep 12, 2018 •

edited

Loading