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

flag plattform specific directory separators #260

Open
jand271 opened this issue Aug 2, 2022 · 8 comments
Open

flag plattform specific directory separators #260

jand271 opened this issue Aug 2, 2022 · 8 comments
Labels
component: sem Affects semantic analysis difficulty: high This change will be tricky or large tool: mh_lint Affects the linter

Comments

@jand271
Copy link

jand271 commented Aug 2, 2022

What kind of feature is this?

  • New feature in MISS_HIT

Your MATLAB/Octave environment

  • MATLAB
  • r2022a

MISS_HIT component affected
Choose one or more of the below:

  • Code metrics

Describe the solution you'd like
I'm working at a company with code repo among many developers that needs to work on windows and linux. The developers keep specifying path construction functions manually (e.g., 'example/path' instead of using fullfile). This is driving me crazy and causing bugs everywhere.

It would be nice if miss_hit would say "I found a '/' or '\'. Use fullfile instead".

This would force engineers to properly specify paths.

@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 9, 2022

As much as I would like to have this type of feature, my hunch is that this would be hard to implement but I may be wrong.

Will let @florianschanda give his take on this.

@florianschanda
Copy link
Owner

First, I apologise for the delay in reply.

Second, I am not sure I understand fully what you're asking for. Are you trying to avoid \ appearing in parameters to functions such as cd?

So for example, is this what you want?

cd C:\potato
     ^ don't do this you dirty windows user you

If so, then this is hard/fragile. First, I need to understand which functions are path manipulating, and second I need to identify which of them actually are and which ones were overloaded/overwritten by the user (which I cannot do yet).

Then even IF semantic name resolution works, then what about this:

random_var = "C:\potato"
cd(random_var);

I will close this ticket as won't do under that assumption. If I got that assumption wrong, please re-open with a comment.

@florianschanda florianschanda added wontfix This will not be worked on component: sem Affects semantic analysis tool: mh_lint Affects the linter labels Aug 9, 2022
@Remi-Gau
Copy link
Contributor

Remi-Gau commented Aug 9, 2022

Will let @jand271 correct me I am wrong but I assumed he meant doing things like this:

this_dir = pwd();
my_file = [this_dir, '/', 'foo',  '/',  'bar', '/',  'my_file.txt']

where as the following is preferable

this_dir = pwd();
my_file = fullfile(this_dir,  'foo',   'bar',  'my_file.txt')

@florianschanda
Copy link
Owner

Yeah, that's even worse than what I imagined :D

MISS_HIT is actually really dumb still. It is 100% syntactic, there is no semantic resolution or interpretation done. You can get really far on that basis (as is evident) but stuff like this is (currently) beyond its capabilities.

@florianschanda
Copy link
Owner

@jand271 for reference, if you could submit a few examples here; just so I have something on record and can add the testsuite... that would be appreciated. Because if I do get sem done; then this check could be implementable.

@jand271
Copy link
Author

jand271 commented Aug 15, 2022

Yes, @Remi-Gau, that is exactly what I'm talking about. Dealing with nightmare at work with this.

I am definitely not familiar with how difficult it is to create tools like MISS_HIT or MISS_HIT's internals, but perhaps MISS_HIT can catch all / and \ within "" (to filter out / and \ in comments) and require fullfile.

This regex expression catches them in my code base...but idk how to get regex to highlight all of the / (as apposed to a single one...but I guess it is good enough).

\'*[//,\\]*\'

Examples:

gunzip([target_dir '\' serverName]);
[obsDir 'mgex-obs/'];
pathNameFormat = [settings.dir '/%d/%03d/'];
path = [basedir 'nav-daily/'];

If MISS_HIT doesn't add this...then I might need to make a custom checker in bash before the code kills someone lol

@florianschanda
Copy link
Owner

OK, I will re-open as I think we can do something in MH Lint maybe. It won't be perfect, but it might catch a few of these.

@florianschanda florianschanda added difficulty: high This change will be tricky or large and removed wontfix This will not be worked on labels Aug 20, 2022
@florianschanda florianschanda changed the title Make everyone purge '/' and '\' in favor of fullfile flag plattform specific directory separators Aug 20, 2022
@RodneyRichardson
Copy link

I believe / is a valid cross-platform path separator. This would only need to flag \ as a potential cross-platform issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sem Affects semantic analysis difficulty: high This change will be tricky or large tool: mh_lint Affects the linter
Projects
None yet
Development

No branches or pull requests

4 participants