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

Project autoshim #163

Merged
merged 17 commits into from
Nov 8, 2018
Merged

Project autoshim #163

merged 17 commits into from
Nov 8, 2018

Conversation

benblank
Copy link
Contributor

@benblank benblank commented Oct 12, 2018

This is a speculative implementation of RFC 23.

A notable change with this PR is the grammar for the notion shim command, which has been moved behind the notion-dev feature flag.

  • notion shim => notion shim list
  • notion shim foo => notion shim create foo
  • notion shim foo --delete => notion shim delete foo

Copy link
Contributor

@mikrostew mikrostew left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good to me overall. I just have some thoughts about how the autoshimming errors are displayed to the user, and maybe @dherman can weigh in on that as well.

src/command/shim.rs Show resolved Hide resolved
src/command/shim.rs Show resolved Hide resolved
crates/notion-core/src/tool.rs Show resolved Hide resolved
src/command/use_.rs Show resolved Hide resolved
@benblank
Copy link
Contributor Author

In the call this morning, Michael raised a corner case I don't think the PR currently covers: running notion use before dependencies are installed.

@benblank
Copy link
Contributor Author

Missing dependencies are no longer reported as errors.

Copy link
Contributor

@mikrostew mikrostew left a comment

Choose a reason for hiding this comment

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

LGTM! still needs review from @dherman

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Looks awesome! I had one tiny nit but I can just do a followup commit.


/// Convert dependency names to the path to each project.
fn get_dependency_path(&self, name: &String) -> PathBuf {
// TODO(158): Add support for Yarn Plug'n'Play.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: the convention we use is // ISSUE(158): .

let errors = project.autoshim();

for error in errors {
display_error(&error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'm starting to doubt the part of the exit codes RFC that says that we should preserve the exit code of the underlying tool even if extra logic afterwards fails. We don't need to block this PR on this, but I filed an RFC issue to follow up.

@dherman dherman merged commit a3952df into volta-cli:master Nov 8, 2018
@dherman
Copy link
Collaborator

dherman commented Nov 8, 2018

I pushed a change to the one nit (// TODO ==> // ISSUE).

@benblank benblank deleted the project-autoshim branch November 14, 2018 18:32
@rwjblue rwjblue mentioned this pull request Dec 13, 2018
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.

3 participants