Skip to content

Conversation

@xianminx
Copy link

No description provided.

@ncalexan
Copy link
Member

Thanks for the patch! I'm terribly busy, just coming off a few vacation days and catching up, but I'll try to process this within a week.

Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

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

Some general comments.

  1. Please include a note about this task in README.md.
  2. Does this, in fact, work? I recall that the plugin sets the Cargo target directory based on the architecture. Does running cargo clean actually remove everything?
  3. Does cargoClean${ARCHITECTURE} make sense? My preference is to generalize the cargo command invocation framework to support clean but also test and friends.
  4. Finally, could you try to add a simple automated test that this in fact works? I'm aware that this adds to the work, but I'd like to improve the testing story as we add features.

I'm marking this as "Request changes" simply for the README.md request, which needs to be done before landing. But I'd like to get some discussion and hopefully some progress on the other questions and testing.

Thanks again!

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