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

Fire fish_postexec event on tokenization error #6880

Merged
merged 3 commits into from Apr 25, 2020

Conversation

PerBothner
Copy link
Contributor

Fixes issue #6816 "shell integration with tokenization error"

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

CHANGELOG.md note could be:
A tokenization error (such as command-line containing just )) now raises the fish_postexec event, as documented.

@faho
Copy link
Member

faho commented Apr 11, 2020

So I've been thinking about this, and I think it would be nicer to introduce a new event if there's an error.

The reason being that it's not entirely clear that postexec handlers would be useful if nothing was actually executed.

Your function would then just register for both events.

@PerBothner
Copy link
Contributor Author

PerBothner commented Apr 11, 2020

A distinct signal for error would also work. Wold it be only for tokenization errors (corresponding to my patch), or also other kinds of errors (which would require other changes)?

Based on share/functions/__fish_config_interactive.fish and share/functions/fish_vi_cursor.fish I'd say fish_postexec should be called if any only if fish_preexec has been called.

@faho
Copy link
Member

faho commented Apr 11, 2020

A distinct signal for error would also work. Wold it be only for tokenization errors (corresponding to my patch), or also other kinds of errors (which would require other changes)?

I would imagine all kinds of errors that cause us not to execute a command - I think that might be

  • tokenization (i.e. the syntax is wrong)
  • empty variable-as-command - e.g. $PAGER file if $PAGER is unset
  • empty globs - ls *.nonexistent

But I think for now just the tokenization will suffice.

@ridiculousfish ridiculousfish requested review from ridiculousfish and removed request for ridiculousfish April 12, 2020 23:19
Copy link
Member

@ridiculousfish ridiculousfish left a comment

Choose a reason for hiding this comment

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

In addition to faho's changes, please move the event posting to outside the reader_shell_test call (which is supposed to only do syntax validation). Here is where fish acts on the test result.

@PerBothner
Copy link
Contributor Author

Ok - I moved the event_fire_generic call as suggested (which I also initially considered) and renamed it to fish_posterror.

However, this is still one (less serious) problem: The error message is printed while we're still in input mode, so it is printed with the wrong style. Fixing this using heuristics is no doubt possible, but it would be less complex/fragile to have an explicit fish_preerror event. However, I'm not sure where it should go, given that the comment before report_tokenizer_error says "This is a hack".

@faho faho added docs An issue/PR that touches or should touch the docs enhancement labels Apr 25, 2020
@faho faho added this to the fish 3.2.0 milestone Apr 25, 2020
@faho faho merged commit a93ee3a into fish-shell:master Apr 25, 2020
faho added a commit that referenced this pull request Apr 25, 2020
Also remove the "event will be emitted even if the command is invalid"
because it's not the case anymore, AFAICT.

See #6880.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
docs An issue/PR that touches or should touch the docs enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants