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

syntax: Provide default.yaml as fallback definition #2933

Merged
merged 10 commits into from
Apr 18, 2024

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Sep 21, 2023

This will provide a basic rule set which is applied in the moment no known file type can be found or identified. The approach is heavily inspired by nano and his default.nanorc. Now it's possible that files without any extension will be highlighted even they aren't explicitly added to a dedicated syntax definition (e.g. /etc/fstab or config files present under /etc/default/).

Closes #2926

@JoeKar JoeKar marked this pull request as draft September 21, 2023 19:22
@JoeKar JoeKar force-pushed the feature/default-syntax branch 2 times, most recently from 1cade4e to 489d1d7 Compare September 23, 2023 10:58
@JoeKar JoeKar marked this pull request as ready for review September 23, 2023 11:01
@Andriamanitra
Copy link
Contributor

I like the idea but I think your default.yaml is doing too much. A lot of the time when I'm editing a file with an unknown type it's going to be plaintext in which case I don't really want special characters or quotes to be highlighted. Thus I'd prefer to only include rules that are extremely unlikely to match in an unrelated context, similar to the default.nanorc you linked.

Your definition for config categories/sections currently allows newlines which is probably not intended. I would change it to

- identifier: "^\\[[^\\[\\]]+\\]$"

I would also add the spaces before tabs highlighting from default.nanorc:

- error: " +\\t+"

@JoeKar
Copy link
Collaborator Author

JoeKar commented Oct 1, 2023

I like the idea but I think your default.yaml is doing too much. A lot of the time when I'm editing a file with an unknown type it's going to be plaintext in which case I don't really want special characters or quotes to be highlighted. Thus I'd prefer to only include rules that are extremely unlikely to match in an unrelated context, similar to the default.nanorc you linked.

That's something which can be discussed, because it's something tightly coupled to personal preferences. When I edit "unknown" file types then these are quite often configuration files present in the /etc/ folder/tree. The usage of assignments and strings are normal there.

Your definition for config categories/sections currently allows newlines which is probably not intended.

Yes, you're right. Should have been used as in the nano rule from the beginning.

I would also add the spaces before tabs highlighting from default.nanorc:

Done.

@dmaluka
Copy link
Collaborator

dmaluka commented Oct 15, 2023

When I edit "unknown" file types then these are quite often configuration files present in the /etc/ folder/tree. The usage of assignments and strings are normal there.

Wondering if that could be addressed with something like this in settings.json:

    "/etc/*": {
        "filetype": XXX
    }

Maybe not, but you can also always resort to some Lua scripting.

@dmaluka
Copy link
Collaborator

dmaluka commented Oct 15, 2023

I would also add the spaces before tabs highlighting from default.nanorc:

There is a PR #1897. We might still hope it will be merged some day. If it will, and if we even go ahead and make those hltaberrors and hltrailingws options enabled by default, then spaces before tabs (and other similar errors) will be highlighted automatically for all file types, so this error: " +\\t+" will be superfluous.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Oct 15, 2023

Wondering if that could be addressed with something like this in settings.json:

But then we add for every location again a new settings entry, like path x and then y etc. pp.
Let us try to watch from a different perspective:
micro has/provides the capability with the syntax option to be active by default and advertises his self to be...

[...] micro aims to be somewhat of a successor to the nano editor [...] Here is a picture of micro editing its source code. {syntax highlighting screenshots attached}

...so why not doing more or less the same as nano does and provide and use a default template in the moment nothing other fits? We don't need to define other fancy paths/locations, we just have or could have a simple default template, which can be maintained as we do with the rest. The workflow doesn't need to be changed. No additional Lua plugin needs to be involved.

Otherwise I expect some more "issues" being opened like the one I try to fix with this feature. 😄

@dmaluka
Copy link
Collaborator

dmaluka commented Oct 15, 2023

I'm not against a default highlighting as such, I was merely commenting on your specific use case of editing various configs files in /etc, for which you might want a richer highlighting than what is suitable for a default template (i.e. for arbitrary plain-text files), as @Andriamanitra pointed out.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Oct 16, 2023

Ah, ok. Then I got you wrong yesterday...sorry for that.
Then we've already two votes against the special characters and the quotes.

Any suggestions what else shall be removed or added?
I assume, that the configuration section isn't needed as well, since we should focus on plain text only.

this error: " +\t+" will be superfluous.

Then we shouldn't add it from the beginning, am I right? Otherwise it needs to be removed afterwards, in case #1897 is merged.

Edit:
Reduced it to a bare minimum.
Please add further wishes or complains.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Nov 1, 2023

Wondering if that could be addressed with something like this in settings.json:

I used your tip for now and set the file type of these files to ini, because this will give me the necessary highlighting I would expect for the files in this particular directory.
Maybe this PR could become handy for different scenarios. :)

@BarbzYHOOL
Copy link

nice, i would have added a screenshot to illustrate tho

@JoeKar
Copy link
Collaborator Author

JoeKar commented Mar 12, 2024

Are there any additional ideas, hints or suggestions to add for the default/unknown definition?

@zeroarst
Copy link

zeroarst commented Mar 16, 2024

Just came cross this issue. Would like see default syntax configuration. For example sudo nano /etc/ssh/ssh_config ..etc.
Currently, is it possible to define a default one by myself? I think, at least it can pass comment symbol # would be enough for me.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Mar 16, 2024

Overriding the default will work earliest after the PR has been merged. As alternative you can create your explicit user defined highlighting for ssh_config etc.

runtime/syntax/default.yaml Outdated Show resolved Hide resolved
runtime/syntax/default.yaml Outdated Show resolved Hide resolved
internal/buffer/buffer.go Outdated Show resolved Hide resolved
break
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about de-duplicating the code? The size of UpdateRules() is getting scary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I started the refactoring in #3206. This PR will benefit of it too in the moment of necessary rebase.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My work on #3208 made me realize that such refactoring is probably is not an easy thing to do, unfortunately. The logic in UpdateRules() is quite subtle, it's not obvious to me how to abstract any important pieces of it into separate functions without loss of functionality and correctness.

Perhaps we should approach this from a different direction: instead of trying to restructure the code, just add some helpers to the syntax parser API, e.g. ParseDefFromFile() (which would internally just call ParseFile() and then ParseDef()), maybe also FindRuntimeFile() and FindRealRuntimeFile() or something like that, to make the code of UpdateRules() at least shorter and a bit easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

[...] just add some helpers to the syntax parser API, e.g. ParseDefFromFile() (which would internally just call ParseFile() and then ParseDef()), maybe also FindRuntimeFile() and FindRealRuntimeFile() or something like that, to make the code of UpdateRules() at least shorter and a bit easier to read.

Yep, this will reduce the duplication of load/parses and logs in that way that they aren't spread all over UpdateRules().
I will take care of this in the moment #3208 is merged, because then the whole change needs a rebase and I will drop some lines I did.

This is necessary since DoEvent() isn't called in a loop like in the main
application, but as one-shot only and a async draw event can lead to ignore
the explicit injected events.
Additional checks have been added to check the presence of the expected buffers.
internal/buffer/buffer.go Outdated Show resolved Hide resolved
runtime/syntax/default.yaml Outdated Show resolved Hide resolved
pkg/highlight/parser.go Outdated Show resolved Hide resolved
pkg/highlight/parser.go Outdated Show resolved Hide resolved
internal/buffer/buffer.go Outdated Show resolved Hide resolved
internal/buffer/buffer.go Outdated Show resolved Hide resolved
internal/buffer/buffer.go Outdated Show resolved Hide resolved
internal/buffer/buffer.go Outdated Show resolved Hide resolved
…okup

It needs to be processed earliest in the moment no match could be determined.
This will reduce the length of this function and thus improves the
readability.
- `findRealRuntimeSyntaxDef()`
- `findRuntimeSyntaxDef()`

This will reduce the length of this function again and thus improves the
readability.
@JoeKar JoeKar merged commit 0806add into zyedidia:master Apr 18, 2024
3 checks passed
@JoeKar JoeKar deleted the feature/default-syntax branch April 18, 2024 17:38
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.

Syntax highlighting for fstab
5 participants