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

Add alphabetical order check for use statements #7265

Merged
merged 2 commits into from Aug 20, 2015
Merged

Conversation

@tafia
Copy link
Contributor

tafia commented Aug 18, 2015

close #7112

Review on Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Aug 18, 2015

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

@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Aug 18, 2015

-S-awaiting-review +S-needs-code-changes

Thank you for your PR. Unfortunately, this can't land until we have all these failures fixed. Is that something you'd like to work on?


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


python/tidy.py, line 179 [r1] (raw file):
No need for this to be defined here.


python/tidy.py, line 257 [r1] (raw file):
Maybe: use = line[4:] ...


python/tidy.py, line 258 [r1] (raw file):
if match < 0


python/tidy.py, line 266 [r1] (raw file):
There's a sorted() function you can use.


Comments from the review on Reviewable.io

@tafia

This comment has been minimized.

Copy link
Contributor Author

tafia commented Aug 18, 2015

Thanks for the review!
Working on your comments + all these new failures. Should I push all fixes in this pull request?

@Ms2ger Ms2ger added the S-fails-tidy label Aug 18, 2015
@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Aug 18, 2015

Doing it here makes sense to me.

@tafia tafia force-pushed the tafia:tidy-use branch from 4838f5b to d8fa1b8 Aug 19, 2015
@tafia

This comment has been minimized.

Copy link
Contributor Author

tafia commented Aug 19, 2015

Should be all good now ...
I finally sorted the {...} groups as well (it feels better).

I started doing all the fixes manually ... then used this script (my 2nd python script ever!):

import os

from tempfile import mkstemp
from shutil import move
from os import remove, close

def replace_uses(file_path):
    #Create temp file
    fh, abs_path = mkstemp()
    with open(abs_path,'w') as new_file:
        with open(file_path) as old_file:
            uses = []
            for line in old_file:
                if line.lstrip().startswith("use "):
                    uses.append(line[:len(line)-2])
                else:
                    if len(uses) > 0:
                        new_file.write("".join([u + ";\n" for u in sorted(uses)]))
                        uses = []
                    new_file.write(line)
    close(fh)
    #Remove original file
    remove(file_path)
    #Move new file
    move(abs_path, file_path)


# navigate the entire directories and replace uses
for root, subdirs, files in os.walk("./"):
    for f in files:
        if f.endswith(".rs"):
            filePath = os.path.join(root, f)
            replace_uses(filePath)

@Ms2ger let me know if I must squash everything

@Ms2ger Ms2ger removed the S-fails-tidy label Aug 20, 2015
@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Aug 20, 2015

Maybe squash into two commits: first one that fixes all the .rs files, and then on top of that a commit to add the check to the python script.

@tafia

This comment has been minimized.

Copy link
Contributor Author

tafia commented Aug 20, 2015

Fine! I'll try to do it asap.

@tafia tafia force-pushed the tafia:tidy-use branch 2 times, most recently from d2b6603 to cb2d3ef Aug 20, 2015
@tafia

This comment has been minimized.

Copy link
Contributor Author

tafia commented Aug 20, 2015

Squashed and rebased (was failing the tests again). @Ms2ger let me know!

@tafia tafia force-pushed the tafia:tidy-use branch from cb2d3ef to 36c6205 Aug 20, 2015
@tafia tafia force-pushed the tafia:tidy-use branch from 36c6205 to 009f6f6 Aug 20, 2015
@Ms2ger

This comment has been minimized.

Copy link
Contributor

Ms2ger commented Aug 20, 2015

Thanks!

@bors-servo r+ p=1000

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 20, 2015

📌 Commit 009f6f6 has been approved by Ms2ger

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 20, 2015

⌛️ Testing commit 009f6f6 with merge a5fbb2f...

bors-servo pushed a commit that referenced this pull request Aug 20, 2015
bors-servo
Add alphabetical order check for use statements

close #7112

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7265)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Aug 20, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3

@tafia

This comment has been minimized.

Copy link
Contributor Author

tafia commented Aug 21, 2015

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.