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

feat(DS-322): add remove command for remove TVM project #60

Merged
merged 4 commits into from
Dec 22, 2022

Conversation

Henrrypg
Copy link
Contributor

@Henrrypg Henrrypg commented Dec 14, 2022

Description

This PR is to add a command to remove TVM projects.

Also solves #48

Jira

DS-322

How to test

  1. Install TVM on this branch
  2. Create TVM project: tvm project init test v13.3.1
  3. (Optional) You can init the same project in other path and move or rename it to test the messages.
  4. Run tvm project remove v13.3.1@test or tvm project remove v13.3.1@test --prune
  5. Your .tvm in project folder, and root in ~/.tvm/{version}@{name} should be deleted. (With --prune it should delete all project folder)
  • Test functionality
  • Review changes in files

@Henrrypg Henrrypg force-pushed the hpg/command_to_remove1 branch 4 times, most recently from 8e9e58c to cc5b10f Compare December 14, 2022 20:00
@Henrrypg Henrrypg force-pushed the hpg/command_to_remove1 branch 3 times, most recently from 06506ca to 3006c06 Compare December 15, 2022 15:46
@Henrrypg Henrrypg marked this pull request as ready for review December 15, 2022 15:59
@dcoa
Copy link
Contributor

dcoa commented Dec 16, 2022

This works, I just suggest be more clear about the difference between use the flag or not (for me isn't total clear, I had to test to be clear),
The command to create a new project should be updated in the host to test for this PR tvm project init test v13.3.1 .

@Henrrypg Henrrypg force-pushed the hpg/command_to_remove1 branch 3 times, most recently from 065d30e to 8e01807 Compare December 19, 2022 15:55
@Henrrypg
Copy link
Contributor Author

Thank you @dcoa, i have been correcting the order of the arguments and adding more information about the functionality of this command, you can test it again? the idea is be the clearest as possible.

Copy link
Member

@JuanDavidBuitrago JuanDavidBuitrago left a comment

Choose a reason for hiding this comment

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

Its working well!! I think the documentation is clear to begin to use this command.

dcoa
dcoa previously approved these changes Dec 21, 2022
@MaferMazu MaferMazu linked an issue Dec 21, 2022 that may be closed by this pull request
@MaferMazu
Copy link
Contributor

MaferMazu commented Dec 21, 2022

I tried to remove a project env following the documentation, and I can't do it.
Following the --help instructions, I could, and the correct form to call this functionality, following --help, is: tvm project remove <version>@<project-name>
I think updating the doc and the PR with that information is a good idea. If I am wrong, please let me know; maybe I did something wrong.

On the other hand, I think it's better if we change the Remove a Project to Remove a Project Environment subtitle in the doc to be more explicit about what you will remove if you call this command, because when you use the flag, it is clear. If you don't want to change the subtitle, maybe you can add a short description specifying what will be removed if you don't use the flag.

@JuanDavidBuitrago
Copy link
Member

JuanDavidBuitrago commented Dec 21, 2022

I tried to remove a project env following the documentation, and I can't do it. Following the --help instructions, I could, and the correct form to call this functionality, following --help, is: tvm project remove <version>@<project-name> I think updating the doc and the PR with that information is a good idea. If I am wrong, please let me know; maybe I did something wrong.

On the other hand, I think it's better if we change the Remove a Project to Remove a Project Environment subtitle in the doc to be more explicit about what you will remove if you call this command, because when you use the flag, it is clear. If you don't want to change the subtitle, maybe you can add a short description specifying what will be removed if you don't use the flag.

You're right @MaferMazu, the documentation has to be changed.

@Henrrypg Henrrypg dismissed stale reviews from dcoa and JuanDavidBuitrago via e27fb43 December 21, 2022 13:48
@Henrrypg
Copy link
Contributor Author

Thank you @MaferMazu, the PR comment and docs are updated

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

Looks good ✨ Thanks for updating the doc.

@Henrrypg Henrrypg merged commit 12439ff into main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] project remove
4 participants