1369710 - [mozlint] Error if unrecognized path is passed in

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Andrew Halberstadt [:ahal]]

If a user passes an invalid path to |mach lint|, the command will happily ignore it and report 0 errors found. This could mislead the user to thinking that everything is ok when in reality there might be lint errors that went unreported.

Comment 1

[Aditya Addepalli]

(In reply to Andrew Halberstadt [:ahal] from comment #0)
> If a user passes an invalid path to |mach lint|, the command will happily
> ignore it and report 0 errors found. This could mislead the user to thinking
> that everything is ok when in reality there might be lint errors that went
> unreported.

@Andrew
Hello! I am a newcomer! I hope to contribute but I need some help getting things started. You recommended this as a good starting point. Right now, I don't really understand what the bug is.

Comment 2

[Aditya Addepalli]

(In reply to dyex719 from comment #1)
> (In reply to Andrew Halberstadt [:ahal] from comment #0)
> > If a user passes an invalid path to |mach lint|, the command will happily
> > ignore it and report 0 errors found. This could mislead the user to thinking
> > that everything is ok when in reality there might be lint errors that went
> > unreported.
> 
> @Andrew
> Hello! I am a newcomer! I hope to contribute but I need some help getting
> things started. You recommended this as a good starting point. Right now, I
> don't really understand what the bug is.

I did bit of reading, and I understand that linters are used to check for correct coding practices and styles. From what I understand, the path specified for the user is not checked correctly. I am guessing we need to add a couple of lines like
/*
import os.path
os.path.exists(file_path)
*/
to check if the file or directory exists.
Suggestions would be appreciated.
Thank you!

Comment 3

[Andrew Halberstadt [:ahal]]

Hi dyex, yes you are correct! And sorry, I didn't get around to including much context :)

Mozlint is a library that takes a bunch of other linters (either third_party or made by mozilla) and runs them all at the same time. Here are some links:

Source code: https://dxr.mozilla.org/mozilla-central/source/python/mozlint
Linters: https://dxr.mozilla.org/mozilla-central/source/tools/lint
Docs: http://gecko.readthedocs.io/en/latest/tools/lint/index.html

For this bug you won't need to worry about the linters, you'll just be modifying the mozlint library itself. Hint: you probably only need to change the cli.py file. It would also be great to add a test for your change.

Thanks again for your interest! Let me know if you have any questions either around fixing the bug, or submitting patches for review.

Comment 4

[Aditya Addepalli]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Hi dyex, yes you are correct! And sorry, I didn't get around to including
> much context :)
> 
> Mozlint is a library that takes a bunch of other linters (either third_party
> or made by mozilla) and runs them all at the same time. Here are some links:
> 
> Source code: https://dxr.mozilla.org/mozilla-central/source/python/mozlint
> Linters: https://dxr.mozilla.org/mozilla-central/source/tools/lint
> Docs: http://gecko.readthedocs.io/en/latest/tools/lint/index.html
> 
> For this bug you won't need to worry about the linters, you'll just be
> modifying the mozlint library itself. Hint: you probably only need to change
> the cli.py file. It would also be great to add a test for your change.
> 
> Thanks again for your interest! Let me know if you have any questions either
> around fixing the bug, or submitting patches for review.

Can you please assign me to this bug? I'll work on it asap.
I'll follow up with more questions soon.
Thank you :)

Comment 5

[Aditya Addepalli]

I went to the repositry for the code that I needed access to and change, but when I fork the repository the entrire mozilla/positron repo gets forked which is quite big (~1 GB).

I was following this guide.
https://help.github.com/articles/fork-a-repo/

I never used git much before, but as far as I know, I have to identify where I need to add the required lines and send a pull request to you.

Is this correct? Do I have to clone both repositories (the cloned one and the original one) and then sync them?
After this, am I supposed to make changes to the code a send a Pull request?

I was thinking that there must be a way to fork  mozilla/positron/python/mozlint i.e only the files I require instead of the entire mozilla/positron.

Comment 6

[Aditya Addepalli]

I also do not know who I test the changes I've made. I have built firefox from source but don't know how to proceed from here. Relatd web links would help me a lot.
Thank you

Comment 7

[Aditya Addepalli]

ahal: Please take a look at this https://github.com/mozilla/positron/pull/155
How do I write a test for this? I am trying to use pytest. Do I have to use the function find_linters defined in cli.py and specifiy and example path?

Comment 8

[Andrew Halberstadt [:ahal]]

Hi dyex, in order to write a test you can re-use the 'test_roller.py' test file. Simply add a new function there and copy how the other functions do it. You shouldn't need to make any new files for that. You can run the tests with:
./mach python-test python/mozlint

But your patch isn't checking the right thing, we want to validate the "paths" argument in the "run" function. Also you've found the wrong repository :). The source code lives in a mercurial repo:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code

There is also a read-only git clone if you prefer to use git, but we don't use pull requests and you won't be able to push anything to there:
https://github.com/mozilla/gecko-dev

Normally changes get pushed to a tool called mozreview, and review is conducted there, but for now you can simply upload a patch as an attachment to this bug. I'd also recommend taking a look through our contribution documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide

Comment 9

[Aditya Addepalli]

Attachment: Using os.path.exists to check if the path that is entered by user is a valid one.

I think I understand what I did wrong.

Is this how I add a patch? Is there a particular format for doing so?
I will add the code for testing, if this is correct.

Comment 10

[Andrew Halberstadt [:ahal]]

Not quite, you need to provide a diff, and while uploading check the 'patch' box. The diff needs to include the commit metadata, like author name and patch title. You can use `hg export` to do this in mercurial, see:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I think on git, you could use git show. But please make sure you are rebased on the latest source, that positron repo is likely out of date.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Indeed, the Positron repo is very out-of-date, so it isn't a good repo to use for this change.  You should use mozilla/gecko-dev <https://github.com/mozilla/gecko-dev>.

Once you've cloned that repo, created a branch, and committed your changes to it, generate a patch using `git format-patch -k -p master`.

Also note these recommendations for how to write the commit message <https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions>.

Comment 12

[Aditya Addepalli]

Attachment: Using os.path.exists to check if the path that is entered by user is a valid one.

Please review the above. Sorry for the delay.

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8892723 [details] [diff] [review]
Using os.path.exists to check if the path that is entered by user is a valid one.

Thanks dyex! No worries about the delay.

For the future, to request a review you flip the "review" flag to ? and then put my name in the "Requestee" box (e.g :ahal). Then when I've finished reviewing, I'll either set it to + or -.

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8892723 [details] [diff] [review]
Using os.path.exists to check if the path that is entered by user is a valid one.

Review of attachment 8892723 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! I think putting a 'print' statement in a list comprehension is a syntax error in Python 2 (it will work in python 3 though, but still likely not do what you expect).

You can run the mozlint tests by doing:
./mach python-test python/mozlint

That should give you an idea if the patch is working or not. I also recommend you use `hg export` to generate your patches, e.g:
$ hg export > bug1369710.patch

Then upload the resulting file.

Thanks for sticking with this, you're getting close!

::: patch.py
@@ +222,5 @@
>      lint = LintRoller(**lintargs)
>      lint.read(find_linters(linters))
>  
> +    # Check if the path that is entered is a valid one.
> +    paths = [path if os.path.exists(path) else print('specified path ' + path + ' does not exist') for path in paths]

I'd lets use a list comprehension to get all the missing paths, and then do something like:
print("The following paths do not exist:\n{}".format("\n".join(missing))

afterwards.

Comment 15

[Aditya Addepalli]

Attachment: bug1369710.patch

How do I write a function that checks if the correct path is entered in?

I tried something like:
@pytest.fixture(params=[
    './linters',                  #this path exists
    './linters/structured.yml',   #this path exists
    './files/foobar.js',          #this path exists
    './files/no_such_dir'         #this path doesn't exist
    './files/no_such_file.py'])   #this path doesn't exist
def test_valid_path(request):
    assert(os.path.exists(request.param) == True) # this will give an error

For some reason the test does not run, and I get 'no tests ran in 0.00s'

Comment 16

[Andrew Halberstadt [:ahal]]

I think you want something like:

@pytest.parametrize('path', [
    'path/foo',
    'path/bar'])
def test_valid_path(path):
    ...

Thanks for thinking about a test! But this check isn't to tell whether or not the files checked under 'linters' and 'files' ree exist or not. It's to check whether what the user passes in exists on the command line. For example, when they run:
./mach lint path/to/foo

We're making sure that 'path/to/foo' exists. If you want it's probably fine to skip a test for this, though if you'd like to try and figure out adding a test, please feel free!

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8893659 [details] [diff] [review]
bug1369710.patch

Review of attachment 8893659 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again, almost there!

::: python/mozlint/mozlint/cli.py
@@ +114,5 @@
>      lint.read(find_linters(linters))
>  
> +    # Check if the path that is entered is a valid one.
> +    missing_paths = [path if os.path.exists(path) for path in paths]
> +    print("The following paths do not exist:\n{}".format("\n".join(missing_paths))

This will print the message every time, even if there's no missing tests. You'll want to put this print behind an if statement.

Also, I think we should error out if there are missing paths, so please add a 'return 1' after the print. To be more clear that it's an error, please change the message to "error: following paths do not..."

Comment 18

[Aditya Addepalli]

Attachment: bug1369710.patch

Am I missing anything?

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8894211 [details] [diff] [review]
bug1369710.patch

Review of attachment 8894211 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, the code is good! Just one more thing though, please change the commit message to reflect Mozilla's conventions, as per:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

It should have the bug number, message (please mention it's for mozlint) and r=ahal at the end. Thanks again for your contribution!

Comment 20

[Aditya Addepalli]

Attachment: bug1369710.patch

Thank you for all your help!

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8894874 [details] [diff] [review]
bug1369710.patch

Review of attachment 8894874 [details] [diff] [review]:
-----------------------------------------------------------------

I'll get this landed on your behalf later today, you'll see a message posted here by a bot when it gets merged. Thanks for your contribution!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2c22f4ef01
[mozlint] Ensure that a valid path is entered. r=ahal

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f2c22f4ef01

Comment 24

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/9f2c22f4ef018075e5887125fe72118c2b48800d
Bug 1369710 - [mozlint] Ensure that a valid path is entered. r=ahal