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

Rename m to mount, but keep signature #1226

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Rename m to mount, but keep signature #1226

merged 1 commit into from
Jul 28, 2020

Conversation

stephendolan
Copy link
Member

@stephendolan stephendolan commented Jul 26, 2020

Background and context

  • Initial implementation was named mount, and had a signature of: mount MyComponent.new(name: "Test")
  • Rename of mount -> m occurred, which also changed the method signature to: m MyComponent, name: "Test"
  • Rename m to mount, but keep signature #1221 opened after several issue discussions about which direction to take, deciding to revert the rename but maintain the new signature.

Purpose

This PR renames m -> mount, retains the method signature as-is, and removes the old signature completely.

Closes #1221

Description

I believe I caught everything necessary. I used the regular expressions \bm[\s(]+[A-Z] to locate the items that needed to be changed. The only things I'd call out that need explicit review:

  • I wasn't sure how to handle the old signature. Do we want deprecation of m, mount with the old signature, or do we just revert to mount with the new signature to get any confusion sorted during the next version bump?
  • I wasn't sure whether updating the UPGRADE.md file was allowed, but it seemed prudent to let anyone migrating from 0.22 -> 0.23 know that they don't really need to rename mount -> m.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@stephendolan stephendolan changed the title Revert mount rename to m Rename m to mount, but keep signature Jul 26, 2020
Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

A couple small suggestions. Thanks for the PR @stephendolan!

UPGRADE_NOTES.md Show resolved Hide resolved
src/lucky/mount_component.cr Show resolved Hide resolved
@jkthorne
Copy link
Contributor

should we keep the m alias?

@stephendolan
Copy link
Member Author

@wontruefree ,

There have been a couple issue/PR comment threads that I've read through. I was going by the comment @paulcsmith left here: #1049 (comment)

I think the idea is that we don't want multiple ways to mount components since m is hard to search for, and may not intuitively be linked to mount. The option I read for handling a shorter mount call would be to implement an alias in your application.

@stephendolan
Copy link
Member Author

@paulcsmith Added back the UPGRADE_NOTES that I removed, but left the note that we reverted it. If you don't mind running through src/lucky/mount_component.cr one last time to make sure I added those deprecations correctly, I'd appreciate it!

Tests are all passing, though, so this should be good to merge!

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Looks great!

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.

Rename m to mount, but keep signature
3 participants