-
Notifications
You must be signed in to change notification settings - Fork 1.3k
"unknown-token" is overloaded #695
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
Comments
Before this commit, the command word was highlighted as "unknown-token" not because alias loops are invalid, as a comment incorrectly claimed, but because the command word «a» resolved to a «b» that was ineligible for being expanded as an alias, and there was no function/builtin/etc. called "b". Add a function "b" to demonstrate that alias loops are valid. I've also filed issue #695 about the overloading of "unknown-token".
I thought there was already an issue for this, but perhaps all the discussion was on IRC. I think we need at least four new styles: Unopposed to further splitting past those so that each error style is distinct. |
Sounds good to me. Labelled "good first issue". |
Triage: incomplete, new feature (as opposed to bugfix), but referenced by many other issues so presumed a popular feature request, 0.8.0 has plenty of content already merged, redrawhook is the next priority: remilestone 0.9.0. |
Highlighting all errors the same way makes sense from a UI perspective (cough #691 cough), but it makes the tests weaker: when a test checks that an alias is highlighted as
unknown-token
, it can't easily check whether that's due to a syntax error (as inalias x='() ]]'
) or due to, say, a command word not being an existing alias/function/etc. (as fixed in 9931990).How about adding some new styles for more specific errors — e.g.,
unknown-token-not-a-command-name
,unknown-token-parse-error
, etc — that will be highlighted the same way by default, but will make the tests more specific?The text was updated successfully, but these errors were encountered: