Skip to content

/ servo

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

Report tidy error for space after ( #13361

Merged
merged 1 commit into from Sep 24, 2016
Merged

Report tidy error for space after ( #13361

merged 1 commit into from Sep 24, 2016

Conversation

@mylainos
Copy link
Contributor

mylainos commented Sep 21, 2016 •
edited by larsbergstrom

Add report in tidy for code which have a space after the ( like some_function( argument)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13350 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@highfive
Copy link

highfive commented Sep 21, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.
@highfive
Copy link

highfive commented Sep 21, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/rust_tidy.rs, python/tidy/servo_tidy/tidy.py
@jdm
jdm reviewed Sep 21, 2016
View changes
python/tidy/servo_tidy/tidy.py Outdated
@@ -483,6 +483,7 @@ def check_rust(file_name, lines):
(r"\{[A-Za-z0-9_]+\};", "use statement contains braces for single import",
lambda match, line: line.startswith('use ')),
(r"^\s*else {", "else braces should be on the same line", no_filter),
(r"(fn )?[A-Za-z0-9_<>]+\( [A-Za-z0-9]", "extra space after (", no_filter),

This comment has been minimized.

Sign in to view
@jdm

jdm Sep 21, 2016 •
edited

Member

What happens if we just match \(\s without any of the surrounding characters? Do we get false positives?

This comment has been minimized.

Sign in to view
@mylainos

mylainos Sep 22, 2016

Author Contributor

Yes, if you consider macro false positive.
Example:

macro_rules! declare_viewport_descriptor {
    ( $( $variant: ident($data: ident), )+ ) => {
        declare_viewport_descriptor_inner!([] [ $( $variant($data), )+ ] 0);
    };
}

This comment has been minimized.

Sign in to view
@mylainos

mylainos Sep 22, 2016

Author Contributor

With [^$]\(\s there is only one error due to macro (here):

macro_rules! thread_types ( ( $( $fun:ident = $flag:ident ; )* ) => (
@wafflespeanut
Copy link
Member

wafflespeanut commented Sep 22, 2016

Also, please update the files once you're done :)
@wafflespeanut
wafflespeanut reviewed Sep 22, 2016
View changes
components/style/thread_state.rs Outdated
@@ -22,7 +22,7 @@ bitflags! {
}
}

macro_rules! thread_types ( ( $( $fun:ident = $flag:ident ; )* ) => (
macro_rules! thread_types (($( $fun:ident = $flag:ident ; )* ) => (

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Sep 22, 2016

Member

I'm not sure whether the spaces should be fixed for this macro. It had a valid style, I believe?

This comment has been minimized.

Sign in to view
@mylainos

mylainos Sep 22, 2016

Author Contributor

I'm agree with you and I've find a fix !
@wafflespeanut
wafflespeanut reviewed Sep 23, 2016
View changes
python/tidy/servo_tidy/tidy.py Outdated
@@ -483,6 +483,7 @@ def check_rust(file_name, lines):
(r"\{[A-Za-z0-9_]+\};", "use statement contains braces for single import",
lambda match, line: line.startswith('use ')),
(r"^\s*else {", "else braces should be on the same line", no_filter),
(r"[^\(][^$]\([ \t\r\v\f][^\(]", "extra space after (", no_filter),

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Sep 23, 2016

Member

We don't have to explicitly make it ignore macros. This could be a bit more simpler. Since we're concentrating on function names, we can assume that there shouldn't be any space or $ before the parentheses. Also, we don't want the vertical space, form feed and carriage return checks - just the space and tab should be fine :)
@wafflespeanut wafflespeanut assigned wafflespeanut and unassigned nox Sep 23, 2016
@wafflespeanut
Copy link
Member

wafflespeanut commented Sep 23, 2016

Please squash the commits and we'll be good to go :)
@mylainos
Issue #13350 Report tidy error for space after (
Loading status checks…
0ac212a
@mylainos
Copy link
Contributor Author

mylainos commented Sep 23, 2016

Done !
@wafflespeanut
Copy link
Member

wafflespeanut commented Sep 23, 2016

Thanks!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2016

📌 Commit 0ac212a has been approved by Wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2016

⌛ Testing commit 0ac212a with merge 2ba804e...
bors-servo added a commit that referenced this pull request Sep 24, 2016
@bors-servo
Auto merge of #13361 - Mylainos:issue-13350, r=Wafflespeanut
Loading status checks…
2ba804e 
Report tidy error for space after (

<!-- Please describe your changes on the following line: -->
Add report in tidy for code which have a space after the `(` like `some_function( argument)`

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13350 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13361)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Sep 24, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev
@bors-servo bors-servo merged commit 0ac212a into servo:master Sep 24, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@mylainos mylainos deleted the mylainos:issue-13350 branch Sep 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

@wafflespeanut wafflespeanut

Assignees

@wafflespeanut wafflespeanut

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Report tidy error for space after (
6 participants
@mylainos @highfive @wafflespeanut @bors-servo @jdm @nox
You can’t perform that action at this time.