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

./mach test-tidy --faster shouldn't display anything when the file list is empty #9778

Closed
KiChjang opened this issue Feb 27, 2016 · 16 comments
Closed

Comments

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 27, 2016

keith$ ./mach test-tidy --faster
Checking files for tidiness...
  Progress: 100% (17/17)
Running the WPT lint...

tidy reported no errors.
@jdm
Copy link
Member

@jdm jdm commented Feb 27, 2016

Code: python/tidy.py

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Feb 27, 2016

Why?

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Feb 27, 2016

Because it is not run at all when using the --faster option.

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 27, 2016

Damn, I forgot that! On the brighter side, it's a good first issue :)

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Feb 27, 2016

That was fixed in #9394

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Feb 27, 2016

I just did a git pull from master and I still see the following:

keith$ ./mach test-tidy --faster
Checking files for tidiness...

Running the WPT lint...

tidy reported no errors.
@jdm
Copy link
Member

@jdm jdm commented Feb 27, 2016

@Ms2ger means that the WPT lint was made to run when --faster is used in #9394, so the message is actually correct.

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 27, 2016

@Ms2ger If there are no changes to the WPT files in the diff, then no files are generated, and so the lint doesn't run.

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Feb 27, 2016

In which case, we should instead change the --help message, it says the following for me:

keith$ ./mach test-tidy --help
usage: mach [global arguments] test-tidy [command arguments]

Run the source code tidiness check

Global Arguments:
  -v, --verbose         Print verbose output.
  -l FILENAME, --log-file FILENAME
                        Filename to write log data to.
  --log-interval        Prefix log line with interval from last message rather
                        than relative time. Note that this is NOT execution
                        time if there are parallel operations.
  --log-no-times        Do not prefix log lines with times. By default, mach
                        will prefix each output line with the time since
                        command start.
  -h, --help            Show this help message.
  --debug-command       Start a Python debugger when command is dispatched.

Command Arguments:
  --faster       Only check changed files and skip the WPT lint in tidy
  --no-progress  Don't show progress for tidy
@KiChjang KiChjang changed the title Get rid of "Running the WPT lint..." when using test-tidy --faster Change help message of test-tidy --faster Feb 27, 2016
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 27, 2016

No, if there are changes to the WPT files in the diff, then tidy does pass it to the lint.

@wafflespeanut wafflespeanut changed the title Change help message of test-tidy --faster ./mach test-tidy --faster - don't display anything when the file list is empty Feb 27, 2016
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 27, 2016

Mmm, nope - a better title would be...

@wafflespeanut wafflespeanut changed the title ./mach test-tidy --faster - don't display anything when the file list is empty ./mach test-tidy --faster shouldn't display anything when the file list is empty Feb 27, 2016
@zakorgy
Copy link
Contributor

@zakorgy zakorgy commented Mar 1, 2016

I would like to try this

@KiChjang
Copy link
Member Author

@KiChjang KiChjang commented Mar 1, 2016

Go for it!

@KiChjang KiChjang added the C-assigned label Mar 1, 2016
@zakorgy
Copy link
Contributor

@zakorgy zakorgy commented Mar 2, 2016

I got a little confused, with the --faster should the tidy-test skip the WPT lint in tidy and display nothing if the file list is empty, or just skip the WPT lint in tidy as in the --help message?

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Mar 3, 2016

@zakorgy the former - don't display the messages when the file list is empty. For example, if the WPT lint gets an empty list, then we shouldn't show "Checking WPT..." message

@jdm jdm added the C-has open PR label Mar 30, 2016
bors-servo added a commit that referenced this issue Apr 5, 2016
Fix ./mach test-tidy --faster issue

 issue #9778

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10074)
<!-- Reviewable:end -->
@jdm
Copy link
Member

@jdm jdm commented Apr 5, 2016

This should be fixed by #10074.

@jdm jdm closed this Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.