search

mathworks / MATLAB-Language-grammar

This repository contains a regular expression based language grammar for MATLAB to be used by GitHub Linguist for highlighting MATLAB code on GitHub
49 stars 17 forks source link

Fixes for multiple function definitions #73

Closed watermarkhu closed 6 months ago

watermarkhu commented 9 months ago

This PR fixes #72, when function definitions are declared on multiple lines.

I will add comments in every change of the tmLanguage describing why is is required.

The PR adds scope checks and additional cases to the existing test test/t41FunctionDefinitions.m

Limitation

Due to the nature of the per-line matching behavior of the textmate language grammar, and the almost unrestricted usage of MATLAB's line continuation operator ..., it is impossible to differentiate between:

  1. Function with no output argument and input argument declaration including parenthesis on new line: 

    function funcNoOutputParensOnNewLine ...
  (inputArg)
  disp(inputArg);
end

  2. Function with a single output argument without brackets and equals sign on new line:

    function outputArg ... 
  = funcSingleOutputEqualSignOnNewLine (inputArg)
  outputArg = inputArg;
end

Here, a choice needs to be made to favor one over the other. The current PR favors option 2, for no other reason than that it looks less weird to me than option 1. If option 2 is chosen, the input argument of the example of option 1 will be tokenized as:

(inputArg)
%< punctuation.section.parens.begin.matlab
%^^^^^^^^ meta.parens.matlab variable.other.readwrite.matlab
%        ^ punctuation.section.parens.end.matlab

The added testcase covers option 2. https://github.com/watermarkhu/MATLAB-Language-grammar/blob/f69e90b0d935ad1f39caf6728b6561544af0efa9/test/t41FunctionDefinitions.m#L87-L91

watermarkhu commented 9 months ago

It is quite ironic that line continuations, intended to make the code more readable, makes it so problematic to be read by textmate.

watermarkhu commented 7 months ago

@dklilley This has been open for a while now

dklilley commented 7 months ago

My apologies, I must have missed the notification that this was no longer a draft! I will plan to take a look at this next week.

dklilley commented 7 months ago

Thanks for working on this!

These changes look good to me.

I don't know why the tests aren't running automatically in this pull request, but I ran them locally. I am seeing a failure in the lineContinuations.m snap test. Can you check this? You should be able to run it with npm run testGrammarSnap.

watermarkhu commented 6 months ago

Perhaps I started the PR as draft?

Anyway, all tests are passing on my side, on both node 16 and 18. Perhaps you can trigger the action manually to check whether it passes in CI?

dklilley commented 6 months ago

I believe I had an old version of that test locally. Everything looks good to me. Thanks for making this improvement!