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

Show aliases in suggestions #624

Merged
merged 4 commits into from
Apr 26, 2020
Merged

Show aliases in suggestions #624

merged 4 commits into from
Apr 26, 2020

Conversation

Celeo
Copy link
Contributor

@Celeo Celeo commented Apr 19, 2020

src/justfile.rs Outdated Show resolved Hide resolved
src/justfile.rs Outdated Show resolved Hide resolved
@casey
Copy link
Owner

casey commented Apr 20, 2020

Thanks for the PR! Added some comments inline.

type: added
fixes:
- #445
src/common.rs Outdated Show resolved Hide resolved
Copy link
Owner

@casey casey 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 of very minor comments. Also, this could use an integration test that checks the output is correct.

If you look in tests/integration.rs, you can find a test called show_suggestion, which you can make a copy of that adds an alias and checks that it suggests the alias.

src/justfile.rs Outdated Show resolved Hide resolved
src/justfile.rs Outdated Show resolved Hide resolved
@casey casey self-requested a review April 20, 2020 22:58
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

I forgot to actually hit the Request changes radio button, doing that now, which forces me to leave this content-free comment :P

@Celeo
Copy link
Contributor Author

Celeo commented Apr 26, 2020

Heya,

Anything else I can do for this?

@casey
Copy link
Owner

casey commented Apr 26, 2020

Just an integration test!

A couple of very minor comments. Also, this could use an integration test that checks the output is correct.

If you look in tests/integration.rs, you can find a test called show_suggestion, which you can make a copy of that adds an alias and checks that it suggests the alias.

This will mostly be copy pasta from the existing show_suggestion test, but should check that the actual text output when an alias is suggested is correct.

@Celeo
Copy link
Contributor Author

Celeo commented Apr 26, 2020

Sure thing! I remember seeing that request earlier, but must've hidden it or something.

@casey casey merged commit dc7210b into casey:master Apr 26, 2020
@casey casey mentioned this pull request Apr 26, 2020
@casey
Copy link
Owner

casey commented Apr 26, 2020

Merged! Thank you very much for this!

By the way, I don't actually have a changelog generator set up this repo,, so no commit metadata necessary.

And, I'm still merging with the "squash and merge" button on Github, since I don't (yet!) have a policy of signing all commits on this repo (that's a new thing I'm trying out for Intermodal).

@Celeo Celeo deleted the issue_445 branch April 26, 2020 21:29
@Celeo
Copy link
Contributor Author

Celeo commented Apr 26, 2020

Great, happy to contribute!

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.

None yet

2 participants