Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRewrite the ban-type lint in Python #15649
Conversation
highfive
commented
Feb 19, 2017
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon. |
highfive
commented
Feb 19, 2017
|
Heads up! This PR modifies the following files:
|
Yes. |
| @@ -13,6 +13,7 @@ | |||
| from servo_tidy import tidy | |||
|
|
|||
| base_path = 'servo_tidy_tests/' if os.path.exists('servo_tidy_tests/') else 'python/tidy/servo_tidy_tests/' | |||
| COMPILE_FAIL_PATH = '../../../tests/compiletest/plugin/compile-fail/' | |||
This comment has been minimized.
This comment has been minimized.
wafflespeanut
Feb 19, 2017
Member
We should be adding new tests to this directory since we'll be removing the existing compile-fail tests.
This comment has been minimized.
This comment has been minimized.
zimio
Feb 19, 2017
Author
Contributor
Done, btw now I get this warning:
warning: function is never used: match_ty_unwrap
Should I got ahead and delete match_ty_unwrap?
Seems like a function that could be useful in the future.
wafflespeanut
added
S-needs-code-changes
S-needs-tests
and removed
S-awaiting-review
labels
| # There should be any use of banned types: | ||
| # Cell<JSVal>, Cell<JS<T>>, DOMRefCell<JS<T>>, DOMRefCell<HEAP<T>> | ||
| (r"(\s|:)?Cell<JSVal>", "Banned type Cell<JSVal> detected. Use MutJS<JSVal> instead", no_filter), | ||
| (r"(\s|:)?Cell<JS<.+>>", "Banned type Cell<JS<T>> detected. Use MutJS<JS<T>> instead", no_filter), |
This comment has been minimized.
This comment has been minimized.
wafflespeanut
Feb 20, 2017
Member
As you may notice in the travis failure, this pattern also matches UnsafeCell<Foo>, which is not what we want. You can make sure tidy succeeds in the whole repo by running ./mach test-tidy --all.
This comment has been minimized.
This comment has been minimized.
wafflespeanut
added
S-needs-code-changes
and removed
S-awaiting-review
S-needs-tests
labels
|
Looks great! Can you squash the commits appropriately? (like, one for tidy changes and the other for the tests? squashing everything is also fine) |
|
|
Delete old rust ban lint and move tests to python tidy Fix ban lint regex and fix test
zimio
force-pushed the
zimio:issue-15591-rewrite-ban-type-lint
branch
from
016ed71
to
ebcb15d
|
Thanks! @bors-servo r+ |
|
|
highfive
added
S-awaiting-merge
and removed
S-awaiting-review
S-needs-rebase
S-needs-squash
labels
…flespeanut Rewrite the ban-type lint in Python <!-- Please describe your changes on the following line: --> Rewrite the ban-type lint in Python. Question: Should the old ban-type lint written in rust be deleted? --- <!-- 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 #15591 <!-- Either: --> - [ x] There are tests for these changes <!-- 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/15649) <!-- Reviewable:end -->
|
|
zimio commentedFeb 19, 2017
•
edited by larsbergstrom
Rewrite the ban-type lint in Python.
Question: Should the old ban-type lint written in rust be deleted?
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is