Makefile colorizing improvement: colorize at the beginning of line, handling the '\' symbol and other. #15
Conversation
|
Could somebody review? Thank you! |
|
@sorbits, @alkemist, @bradchoate, @infininight, As I see your names in TextMate project, could anybody pay attention to this pull request? Thank you! |
|
@fadeevab I'll be the one reviewing the pull request. This may a little while however as Makefiles to no have any language spec that I've ever found. So deducing what is allowed and where is a bit hit or miss. There are also a few other issues I am already aware of so I will likely do a larger grammar review as I review the PR. |
|
@infininight , It could be helpful that there are some kind of syntax blocks in https://www.gnu.org/software/make/manual/make.html, e.g. for variable: It's spread through the document though. |
|
@infininight, Could you just make simple test to check makefile colorizing with this patch and, possibly, to approve? My point: I assume you have no enough time to make detailed review, while the current patch doesn't make colorizing worse, and even after commit more cases handling is needed, and I will push more PR. So need to move forward. Thank you for your attention. |
|
@infininight, ...just reminding about review. Thanks |
|
@sorbits, @alkemist, @bradchoate, @infininight, Hi, there! Initially I prepared the pull request into VS Code repository, but VS Code team refuses my patch until TextMate approves current pull request. Reason: to not create individual fork of colorizing, and this is a good point. Could you, please, review and approve this patch? Thank you! |
|
Can you break this into separate discrete commits? Want to separate the changes into their purpose. A couple aren't making sense. |
|
Ok, I can. |
|
@infininight, Could you, please, suggest me how to test this XML in Ubuntu? Thank you! |
|
The only thing I am aware of is the Lightshow tester from Github: https://github-lightshow.herokuapp.com This is limited though as it only shows a subset of scopes and there is no way to see them directly. If it's an issue I can piece them out myself but will need to ask some questions likely to do so. |
|
@infininight, Thanks, I'll check it. |
|
@infininight , Finally I found the time to check https://github-lightshow.herokuapp.com, and unfortunately, it doesn't even react for my custom requests, it just stuck at the GET request. |
|
Checked with their tests here. Still their tests don't contain the coverage of the issues which are fixed with my patch ($(info ...) at the beginning of the line and so on). P.S.: I just mean you will not see the difference there before and after my fixes. P.S.2: It doesn't matter, just see my fixes )) |
…example, in the expression "var := $(var) $(info Hello!)" the "info" must be colorized. "variable" pattern is $$(....). Replace \G with (?<=\(), which means "get something right after the open brace which is found in the parent pattern". It makes the deal.
…ized as a continuation of previous). Previous pattern: search until TAB at the beginning of line. New pattern: search until the line without backslash "\" at the end. The "comment" pattern is fixed as well to continue colorizing until the end without a backslash. The patch is combined because comment could be placed in the recipe pattern with some circumstances.
|
@infininight, |
|
@infininight, |
|
I believe one day this pull request will be submitted :) @infininight, This is kindly reminder to review this pull request 👍 Thank you! |
|
Nobody loves makefile :( |
|
@sorbits, @alkemist, @bradchoate, @infininight, Hi, there! Any chances about this pull request? |
…rt contribution to VS Code
|
Up to you, but I strictly recommend you to use my test above and compare with result of my master branch: even if my XML is not perfect from your point of view, it colorizes the test cases as I expect. Thank you for your review! |
I've had to add 2 duplicates of variables definition to properly catch $() and ${}.
The issue is to be in a catched context: $() or ${}. Common regex aka [)}] doesn't work: it incorrectly catches the cases $(}, or ${), or $(var}xxx), and other complex examples.
So, code duplication is the worst thing ever, but seems like there are no better ways currently in TextMate + makefile situation.
…ble and value ("var:=value").
Problem: In the sentense kind of "variable:=value" the symbol ':' is highlited as a part of "variable:".
Cause: [^\s]+ is catched down to '=', dropping the ':=', '?=' and other cases.
Measure: restrict '=' to be a finisher of [^\s]+ in a case of [?:+!] behind one, allowing to select other assign operators.
Cause: Preparing to add the real "recipe" pattern which starts from the TAB symbol. Issue: #1.
Problem: In the recipe "<tab>echo '#'message" hastag and message behind is highlighted as a start of a comment. Cause: 'recipe' patterns are not enabled. Measure: Create a 'recipe' pattern starting with a [\t] symbol. Remove inactive matching. Issue: #1.
Makefile syntax doesn't imply shell inside backquotes ``.
Problem: 1. 'source.shell' hides coloring of Makefile variables, which must have a higher priority; 2. incorrect coloring when there is no space before a shell: (a) <tab>@echo Test, (b) var!=echo 134 Cause: source.shell catches the context, it doesn't know about Makefile syntax. Measure: Remove source.shell.
@ suppresses the normal 'echo' of the command that is executed. - ignores the exit status of the command that is execured. + means to execute command always, even under `make -n`.
…(nested-var) := value
Problem: In a recipe "@./some-program" the "." (point) and "/" (slash) are colorized. Cause: regex [@-+] means to colorize "from @ to +". Measure: regex is fixed to [@\-+] to colorize only @,- and +. Test case would be added to Microsoft/vscode.
Fix syntax highlighting for nested directives inside define blocks. While it's not a widely used feature of Make, it is valid to nest define/endef blocks within one another arbitrarily in a makefile (at least in the case of GNU Make). This syntax change fixes highlighting of such nested blocks.
When a recipe contains a line with only a comment, and a prefix precedes the comment, the highlighting is invalid (the comment is not highlighted as such). This allows comments to appear in recipes. Fixes #4
Fix syntax highlighting of comments with prefixes
This reverts commit 8b350e4.
…g to GNU Make documentation. It improves highlighting of statements with double '=' like the following: var:=$(val:.c=.o) "A variable name may be any sequence of characters not containing ‘:’, ‘#’, ‘=’, or whitespace." See https://www.gnu.org/software/make/manual/html_node/Using-Variables.html.
Below line used to be interpreted as Make target with the name "export a ?= b". Treat it as directive since this is what Make does. export a ?= b:c
Below line used to be interpreted as Make target with the name export a ?= b. Treat it as directive since this is what Make does. export a ?= b:c
It fixes "Wrong tokenization of 'else' statement for ifdef in Makefile #10".
It fixes "Makefile syntax highlighting partially breaks when a # is in a string #9".
It is becuase the comment in the recipe is a part of Shell syntax, not the part of Makefile syntax. It fixes "Makefile: incorrect syntax handling of comments with prefix #4".
Now the following test case should work: ```make ifdef $(var-in-ifdef) # Comment $(info Good, as expected) # Comment else $(var-can-be-here) # "else" is going to be colozired $(error 'Else' is not expected here) # Comment endif # Comment ```
Problem: "#" means a beginning of a comment in ANY place unless it's escaped How it works: var:=blah#actual_comment var:=blah\#not_a_comment var:=blah\\#actual_comment var:=blah\\\#not_a_comment
`debian/rules` files typically start with `#!/usr/bin/make -f` but have a nonstandard filename so they don't match `Makefile`, etc. Co-authored-by: Alexander Fadeev <fadeevab.com@gmail.com>
1). Colorize when the expression at the beginning of line.
2). Handle '\' at the end of prerequisites (the next line should be colorized as a continuation of previous).
3). Colorize (origin|flavor).
4). Fix colorizing of built-in which stands after a common variable.
Note: tested only at JSON version in VS Code. Current patch is ported from JSON to XML and is not verified. I appreciate anybody could help to test this patch. Thank you!