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.

Sign up for GitHub

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

Jump to bottom

chore: Rename --importmap to --import-map #7032

Merged
merged 2 commits into from
Oct 20, 2020
Merged

chore: Rename --importmap to --import-map #7032

merged 2 commits into from
Oct 20, 2020

Conversation

nayeemrmn
Copy link
Collaborator

Closes #6976.

@nayeemrmn
Copy link
Collaborator Author

@bartlomieju Can we land this?

@bartlomieju
Copy link
Member

@nayeemrmn this PR is rather a disruptive change. Is there a huge gain with renaming? Isn't it 2.0-type of change?

@nayeemrmn
Copy link
Collaborator Author

@bartlomieju Disruptive for users? Feedback seems positive and the flag is unstable. This is the only inconsistent flag name before we can consolidate. It's only 26+/26- so can't be that disruptive for other PRs.

@bartlomieju
Copy link
Member

@bartlomieju Disruptive for users? Feedback seems positive and the flag is unstable. This is the only inconsistent flag name before we can consolidate. It's only 26+/26- so can't be that disruptive for other PRs.

Right... We weren't planning on landing this one for 1.4.0. I think we could slate it for 1.5.0.

@bartlomieju bartlomieju added this to the 1.5.0 milestone Sep 11, 2020
@bartlomieju
Copy link
Member

@kitsonk @ry @lucacasonato what is your opinion on this PR? Although I feel the sentiment for the change, it's gonna be a disruptive one. The other option would be to support both --importmap and --import-map while showing only the latter in the help message.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 1, 2020

My opinion hasn't changed since 11th Aug (#6976 (comment)):

As far as renaming... 🤷 but I do like --import-map better.

@caspervonb
Copy link
Contributor

@kitsonk @ry @lucacasonato what is your opinion on this PR? Although I feel the sentiment for the change, it's gonna be a disruptive one. The other option would be to support both --importmap and --import-map while showing only the latter in the help message.

Since it's just one line of clap configuration, think we should keep importmap as a hidden alias until 2.0. (e.g --help shows import-map)

@ry
Copy link
Member

ry commented Oct 1, 2020

I like Bartek's proposal. I'd be happy to rename it in 1.5 if we continue to support the old one.

@nayeemrmn
Copy link
Collaborator Author

Done, clap provides this feature.

@SyrupThinker SyrupThinker mentioned this pull request Oct 11, 2020
Merged
@nayeemrmn nayeemrmn force-pushed the import-map-rename branch 2 times, most recently from 42d1395 to 0889273 Compare October 14, 2020 21:34
@bartlomieju
Copy link
Member

bartlomieju commented Oct 20, 2020 •
edited
Loading

@nayeemrmn please rebase, I'll land this PR tonight Actually managed to do that myself

bartlomieju
bartlomieju approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @nayeemrmn

@bartlomieju bartlomieju merged commit 070d996 into denoland:master Oct 20, 2020
@nayeemrmn nayeemrmn deleted the import-map-rename branch October 20, 2020 13:07
@nnmrts nnmrts mentioned this pull request Nov 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caspervonb caspervonb caspervonb approved these changes

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

Rename --importmap to --import-map
5 participants
@nayeemrmn @bartlomieju @kitsonk @caspervonb @ry