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

Missing an obvious alias. #2

Merged
merged 4 commits into from
Apr 6, 2022
Merged

Conversation

richardpark-msft
Copy link
Contributor

@richardpark-msft richardpark-msft commented Feb 1, 2022

This repo seems to handle some small values, but lacks the larger values needed to really hit enterprise levels.

NOTE: updating as per @KarishmaGhiya's comment.

Copy link

@KarishmaGhiya KarishmaGhiya left a comment

Choose a reason for hiding this comment

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

Though your PR is missing a description, I can approve this for you!

@@ -209,6 +209,7 @@ alias grb2="git rebase -i HEAD~2"
alias grb3="git rebase -i HEAD~3"
alias grb5="git rebase -i HEAD~5"
alias grbt="git rebase -i HEAD~10"
alias grbtt="git rebase -i HEAD~1010"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
alias grbtt="git rebase -i HEAD~1010"
alias grbtt="git rebase -i HEAD~$(echo "obase=2;10" | bc)

Copy link
Owner

Choose a reason for hiding this comment

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

Hardcoding values isn't enterprise-y enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I measured this across 10000 runs and your change really drags down the perf. Is this a strict requirement?

Copy link
Owner

@benbp benbp Feb 9, 2022

Choose a reason for hiding this comment

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

Are you the using enterprise version of zsh? If not it will rate limit rc file execution. We only support "zsh for business" subscriptions.

Choose a reason for hiding this comment

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

I agree with @benbp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maorleger - can you clarify?

@richardpark-msft
Copy link
Contributor Author

@chrisradek, I believe you've run into issues with this before, correct?

Copy link

@maorleger maorleger left a comment

Choose a reason for hiding this comment

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

Please make sure we can support all values between 10 and 1010

@benbp
Copy link
Owner

benbp commented Feb 9, 2022

Please make sure we can support all values between 10 and 1010

I would also like us to consider supporting negative values, in order to see what commits we will make in the future. This should rapidly improve our development velocity.

@chrisradek
Copy link

@richardpark-msft aliasing can be hard 😄

@benbp
Copy link
Owner

benbp commented Feb 9, 2022

This needs an integration test.

@richardpark-msft
Copy link
Contributor Author

richardpark-msft commented Feb 9, 2022

This needs an integration test.

I looked but I didn't see an example of that. Is your contributors guide out of date?

@richardpark-msft
Copy link
Contributor Author

@maorleger

Please make sure we can support all values between 10 and 1010

Issue filed: #4

@richardpark-msft
Copy link
Contributor Author

@lmolkova, is it okay if @benbp doesn't have this instrumented properly? TBH, he seems to not care about all of our standards at all.

@lmolkova
Copy link
Contributor

lmolkova commented Feb 9, 2022

should we consider a more future-proof approach so we never confuse anyone with irrelevant history?
alias screwit="git rebase --soft --root && git commit -m "i did it all in one commit"" ?

@richardpark-msft
Copy link
Contributor Author

should we consider a more future-proof approach so we never confuse anyone with irrelevant history? alias screwit="git rebase --soft --root && git commit -m "i did it all in one commit"" ?

Added! Hopefully this will be the feature that sways @benbp to treat this like a real open source project.

.zshrc Outdated Show resolved Hide resolved
Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
@alzimmermsft
Copy link

@maorleger

Please make sure we can support all values between 10 and 1010

Issue filed: #4

I need support for 1011, should I file a new issue or add on to this one?

@lmolkova
Copy link
Contributor

lmolkova commented Mar 28, 2022

@maorleger

Please make sure we can support all values between 10 and 1010

Issue filed: #4

I need support for 1011, should I file a new issue or add on to this one?

@alzimmermsft we track all issues here, just make sure to add a link to the comment explaining your issue in the commit message. You can edit comment as you go to provide more context.

@benbp
Copy link
Owner

benbp commented Apr 1, 2022

@alzimmermsft @lmolkova we may need to cut a new major version for 1011. It may break some users who have taken hard dependencies on this pull request.

@@ -209,6 +209,9 @@ alias grb2="git rebase -i HEAD~2"
alias grb3="git rebase -i HEAD~3"
alias grb5="git rebase -i HEAD~5"
alias grbt="git rebase -i HEAD~10"
alias grbtt="git rebase -i HEAD~1010"
# Thank you @lmolkova!
alias screwit="git rebase --soft --root && git commit -m "i did it all in one commit""

Choose a reason for hiding this comment

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

Suggested change
alias screwit="git rebase --soft --root && git commit -m "i did it all in one commit""
alias screwit="git rebase --soft --root && git commit -m 'i did it all in one commit'"

Choose a reason for hiding this comment

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

@KarishmaGhiya is the hero we needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

@benbp
Copy link
Owner

benbp commented Apr 6, 2022

We did it folks. I will be sending out ship-it awards within the week. Please post your addresses publicly to this pull request thread.

@benbp benbp merged commit 1ba05d6 into benbp:master Apr 6, 2022
@richardpark-msft richardpark-msft deleted the natural-extension branch June 4, 2022 01:03
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.

7 participants