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

Underline every valid entered path #5872

Merged
merged 3 commits into from May 19, 2019
Merged

Underline every valid entered path #5872

merged 3 commits into from May 19, 2019

Conversation

dawidd6
Copy link
Contributor

@dawidd6 dawidd6 commented May 12, 2019

Description

Before patch

Screenshot from 2019-05-12 13-47-57

After patch

Screenshot from 2019-05-12 13-49-12


All credits goes to @floam (#847 (comment)).

Fixes issue #847

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@faho
Copy link
Member

faho commented May 12, 2019

You'll want to adjust the unit tests as well - in src/fish_tests.cpp, where it says that "/dev/null" is highlighted, you'll need to make it so it's param_valid_path instead of just param.

Also you should go through those and see if any other parameter is possibly a valid file (with an absolute path, let's assume you're building fish in-tree or in a build directory), so we don't have the tests failing on some systems.

@dawidd6
Copy link
Contributor Author

dawidd6 commented May 12, 2019

You'll want to adjust the unit tests as well - in src/fish_tests.cpp, where it says that "/dev/null" is highlighted, you'll need to make it so it's param_valid_path instead of just param.

Fixed.

Also you should go through those and see if any other parameter is possibly a valid file (with an absolute path, let's assume you're building fish in-tree or in a build directory), so we don't have the tests failing on some systems.

To be honest I don't know how to achieve that. I would use some help here.

@faho
Copy link
Member

faho commented May 12, 2019

To be honest I don't know how to achieve that. I would use some help here.

Check the tests you just altered. Every highlight_role_t::param could now become a param_valid_path, if that thing could exist as a path. Since we're disregarding relative paths, you just need to check if any of them starts with a "/". I don't think any do, but it's good to check.

@dawidd6
Copy link
Contributor Author

dawidd6 commented May 12, 2019

Okay, now I think I understand what you mean, but dunno how to code it.

@faho faho added this to the fish 3.1.0 milestone May 19, 2019
@faho faho merged commit 0b3bb0e into fish-shell:master May 19, 2019
@faho
Copy link
Member

faho commented May 19, 2019

Merged, thanks!

@dawidd6 dawidd6 deleted the underline_all_valid_paths branch May 19, 2019 08:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants