-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Improve file detection with signature check capabilities #2819
Conversation
@zyedidia |
e179540
to
8c8e092
Compare
@zyedidia: |
@zyedidia: |
Fix #2894 |
8c8e092
to
6e4230f
Compare
Unfortunately this PR didn't hit the v2.0.12, but maybe there is a chance for the v2.0.13. @dmaluka: And in case you're in the right mood, there are more... |
runtime/help/options.md
Outdated
@@ -392,6 +392,11 @@ Here are the available options: | |||
|
|||
default value: `4` | |||
|
|||
* `detectlimit`: if this is not set to 0, it will limit the automatic parsed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO I'd be rather reluctant to adding new options unless we really know there is a real need for it. (I.e. in this particular case, I'd rather stay with the hardcoded value of 100 for the time being.) Or the user will end up with a zillion options and get lost in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I'm more a friend of having the choice rather been patronised. Just imagine a file with a very large comment block or other fancy header (larger than the e.g. hard coded 100) in front of the important decision mark. At the end the guess could be wrong. That was the reason for the option to give the choice to decide between more precision or faster parse time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can easily imagine a large comment block in the beginning of a file. Good point.
Having a choice is great, but I also think we should try our best to make software do the right thing from the beginning, rather than leave this burden to the user.
In this regard, now I'm thinking (independently of the question whether this line limit should be configurable or not): wouldn't it be a good idea to increase the default value from 100 to e.g. 500 right away? I actually doubt it would cause any real slowdown in practice (but if we concerned about that, we can always verify it with profiling, micro now has -profile
command-line flag for that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW do you know how does it work in vim? i.e. how many lines are checked and how is it configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vim does it the same, but the limit(s) per different checks is/are hard coded (see https://github.com/vim/vim/blob/master/runtime/autoload/dist/ft.vim). The idea was good, but I thought going one step ahead is maybe better and we don't want to be Vim...it's micro
. ;)
0f9567a
to
dd0d784
Compare
dd0d784
to
cf90602
Compare
cf90602
to
8c207b6
Compare
@JoeKar regarding your proposal regarding detectindent:
This would mean essentially reimplementing it in Go to make it tightly integrated into micro. I'm not sure it's a good idea: micro's source code is already pretty complicated as it is, so in cases when we are able to separate some complexity away via a plugin (not at the cost of performance, correctness etc), I think we should take advantage of this possibility. It seems that detectindent is actually such a case. But if you have some observations suggesting that having detectindent integrated with filetype detect would have a real performance advantage (e.g. if you can already see a noticeable slowdown caused by detectindent in some cases, or if you have profiling data showing that detectindent time is a considerable fraction of micro startup time or buffer open time), please let me know. |
While reviewing this PR I've found a couple of small bugs in regex error handling for syntax files. Fixed them in #2913. |
8c207b6
to
278f0d7
Compare
Thanks a lot for your enduring review! :) |
No, not till now. It only came to my mind since it does an additional loop on opening the buffer, which could be prevent in case it would use the same loop. So, just an optimistic optimization saving some CPU cycles. Most probably you're right, that the necessary effort isn't worth the result/try...at least so far. And we definitely have larger performance impacts with the syntax highlighting itself, which should receive the higher priority in case of possible optimizations. |
Now that #2913 has been merged, you need to rebase your PR. |
278f0d7
to
90600bb
Compare
90600bb
to
757e6e8
Compare
This allows more complex detection upon regex rules for a certain amount of lines.
757e6e8
to
bea0326
Compare
bea0326
to
3c16df8
Compare
Fixes #3054 |
This allows more complex detection upon regex rules for a certain amount of lines and not just in the first. A similar approach is chosen by VIM as well.
Fix #2041, Fix #2166