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

mh_style --fix breaking a valid (but oddly-formatted) switch statement #298

Open
j-tyr opened this issue Oct 7, 2024 · 1 comment
Open

Comments

@j-tyr
Copy link

j-tyr commented Oct 7, 2024

MISS_HIT Component affected

  • Style checker

Your MATLAB/Octave environment

  • MATLAB
  • R2024a

Your operating system and Python version

  • Windows
  • Python 3.12.6

Describe the bug

I'm working on a codebase that includes the fit_ellipse.m file from fit_ellipse. This includes the following block of code:

switch (1)
case (test>0),  status = '';
case (test==0), status = 'Parabola found';  warning( 'fit_ellipse: Did not locate an ellipse' );
case (test<0),  status = 'Hyperbola found'; warning( 'fit_ellipse: Did not locate an ellipse' );
end

which looks a bit odd (to me at least) but produces 0 warnings under "default" MATLAB settings.

When running mh_style --fix on this file, we get:

switch 1
    case test > 0 status = '';
    case test == 0 status = 'Parabola found';
        warning('fit_ellipse: Did not locate an ellipse');
    case test < 0 status = 'Hyperbola found';
        warning('fit_ellipse: Did not locate an ellipse');
end

which is not valid MATLAB syntax (we get the error "use a newline, semicolon, or comma before this statement" for each status = ... statement).

There's a few things going on at once here in the original code that mh_style is attempting to fix - redundant brackets, and a combination of both commas , and semicolons ; being used to separate statements in a single line. It looks like we're losing the , when we need a newline?

A second run of mh_style --fix actually corrects this:

switch 1
    case test > 0
        status = '';
    case test == 0
        status = 'Parabola found';
        warning('fit_ellipse: Did not locate an ellipse');
    case test < 0
        status = 'Hyperbola found';
        warning('fit_ellipse: Did not locate an ellipse');
end

with a message like:

In third-party/file-exchange/3215-fit_ellipse/fit_ellipse.m, line 191
|         case test > 0 status = '';
|                     ^ style: end statement with a newline [fixed] [end_of_statements]

Is correcting invalid MATLAB syntax within the remit of a formatting tool? I was assuming the second run would just highlight the error in the logs without touching the code.

There's a similar example in this code with an if statement and mixed use of , and ;, where a first run of mh_style --fix introduces a warning that is then fixed in a second run.

Original code:

% make sure coefficients are positive as required
if (a<0), [a,c,d,e] = deal( -a,-c,-d,-e ); end

After one run of mh_style --fix:

% make sure coefficients are positive as required
if a < 0 [a, c, d, e] = deal(-a, -c, -d, -e);
end

After two runs of mh_style --fix:

% make sure coefficients are positive as required
if a < 0
        [a, c, d, e] = deal(-a, -c, -d, -e);
end
@florianschanda
Copy link
Owner

Thank you for this detailed report, I'll have a look

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

No branches or pull requests

2 participants