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

Cleanup tidy for external deps #10653

Merged
merged 8 commits into from Apr 18, 2016
Merged

Cleanup tidy for external deps #10653

merged 8 commits into from Apr 18, 2016

Conversation

@askeing
Copy link
Contributor

askeing commented Apr 16, 2016

fix #10639

This change is Reviewable
askeing and others added 7 commits Apr 16, 2016
@askeing
Add dependency on flake8 base on python/requirements.txt
1eab6ca
@askeing
Add dependency on toml base on python/requirements.txt
c723ac6
@askeing
Add "servo-tidy" entry point
9228408 
- also update requriements "pyflakes" from 0.8 to 0.8.1 due to following issue
```
Traceback (most recent call last):
  File "/Users/Askeing/software/servo/python/_virtualenv/bin/servo-tidy", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3084, in <module>
    @_call_aside
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3070, in _call_aside
    f(*args, **kwargs)
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3097, in
_initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 653, in _build_master
    return cls._build_from_requirements(__requires__)
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 666, in
_build_from_requirements
    dists = ws.resolve(reqs, Environment())
  File "/Users/Askeing/software/servo/python/_virtualenv/lib/python2.7/site-packages/pkg_resources/__init__.py", line 839, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'pyflakes==0.8.1' distribution was not found and is required by servo-tidy
```
@askeing
Check the "tests/wpt/web-platform-tests" folder before loading "tools…
88c94f2 
….lint" module
Maciej Skrzypkowski @askeing
Test for check_spec function, for tidy.py #9152
22d991d
@askeing
Modified test_spec_link of test_tidy for packaged tidy
27bac66
@askeing
bump the minor version of servo_tidy
Loading status checks…
eb9a7e5 
- and add clean folder into Makefile
@highfive
Copy link

highfive commented Apr 16, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/requirements.txt, python/tidy/Makefile, python/tidy/HISTORY.rst, python/tidy/servo_tidy_tests/tidy_self_test.py, python/tidy_self_test/speclink.rs, python/tidy/servo_tidy/tidy.py, python/tidy/setup.py
@askeing
Remove "python/tidy_self_test/speclink.rs"
Loading status checks…
f5754b1 
- it already move to "python/tidy/servo_tidy_tests/speclink.rs"
python/tidy/servo_tidy/tidy.py
returncode = lint.lint(files)
if returncode:
yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status {0}".format(returncode))
if os.path.isdir(wpt_working_dir):

This comment has been minimized.

Sign in to view
Copy link
@wafflespeanut

wafflespeanut Apr 17, 2016

Member

Just being curious, why are we checking whether wpt_working_dir is a directory?

This comment has been minimized.

Sign in to view
Copy link
@askeing

askeing Apr 17, 2016

Author Contributor

The #10639 (comment) has the requirement:

Don't attempt to import wpt tools.lint in https://github.com/servo/servo/blob/master/python/tidy/servo_tidy/tidy.py#L605 unless the tests/wpt/web-platform-tests directory exists in the current project.

I think this requirement is for external projects (which do not have wpt), if there is no wpt folder, then skip the tools.lint import.

This comment has been minimized.

Sign in to view
Copy link
@wafflespeanut

wafflespeanut Apr 17, 2016

Member

Oh, right. We could've just checked os.path.exists 😄

This comment has been minimized.

Sign in to view
Copy link
@askeing

askeing Apr 18, 2016

Author Contributor

I just want to make sure the path exists and is a directory :P
Do I have to modify it to exists()?

This comment has been minimized.

Sign in to view
Copy link
@wafflespeanut

wafflespeanut Apr 18, 2016

Member

No, that's fine :)
@wafflespeanut
Copy link
Member

wafflespeanut commented Apr 17, 2016

r? @edunham
@highfive highfive assigned edunham and unassigned wafflespeanut Apr 17, 2016
@edunham
Copy link
Contributor

edunham commented Apr 18, 2016

Reviewed 1 of 3 files at r1, 1 of 3 files at r2, 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

python/tidy/HISTORY.rst, line 6 [r4] (raw file):
This is OK for now, but it's generally clearer to specify exactly what changed than to just copy the title of the issue that a set of changes fixes. http://keepachangelog.com/ is better at explaining it than I am :)

Comments from Reviewable
@edunham
Copy link
Contributor

edunham commented Apr 18, 2016

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

bors-servo commented Apr 18, 2016

📌 Commit f5754b1 has been approved by edunham
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

⌛ Testing commit f5754b1 with merge 5269734...
bors-servo added a commit that referenced this pull request Apr 18, 2016
@bors-servo
Auto merge of #10653 - askeing:cleanup_tidy_for_external_deps, r=edunham
Loading status checks…
5269734 
Cleanup tidy for external deps

fix #10639

<!-- 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/10653)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Apr 18, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

⌛ Testing commit f5754b1 with merge 4d80e06...
bors-servo added a commit that referenced this pull request Apr 18, 2016
@bors-servo
Auto merge of #10653 - askeing:cleanup_tidy_for_external_deps, r=edunham
Loading status checks…
4d80e06 
Cleanup tidy for external deps

fix #10639

<!-- 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/10653)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit f5754b1 into servo:master Apr 18, 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
@askeing askeing deleted the askeing:cleanup_tidy_for_external_deps branch Apr 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@edunham edunham

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

`tidy` package needs cleanup to work on external deps
6 participants
@askeing @highfive @wafflespeanut @edunham @bors-servo @KiChjang
You can’t perform that action at this time.