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

Make lint run when directories are passed in #9995

Merged
merged 17 commits into from Mar 16, 2018

Conversation

Projects
None yet
5 participants
@teh-f00l
Copy link
Contributor

teh-f00l commented Mar 13, 2018

Response to issue #9879

Allows tester to pass command line arguments to lint specifying directories they want checked


This change is Reviewable

Update lint.py
Response to issue #9879

Allows tester to pass command line arguments to lint specifying directories they want checked

@wpt-pr-bot wpt-pr-bot added the infra label Mar 13, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Mar 13, 2018

@jgraham
Copy link
Contributor

jgraham left a comment

This looks like a great start, but some changes are required before it can land.

r = os.path.realpath(wpt_root)
paths = [os.path.relpath(os.path.realpath(x), r) for x in kwargs["paths"]]
#make new list in case there are multiple added directories
AllPaths=[]

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

Use snake_case for variable names.

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 13, 2018

Author Contributor

looking into this

AllPaths=[]
for path in kwargs.get("paths"):
# create extension for called directory and merge it with the existing path
extension= path+'/'

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

Use a space before and after operators like =.

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 13, 2018

Author Contributor

kk

for path in kwargs.get("paths"):
# create extension for called directory and merge it with the existing path
extension= path+'/'
newPath=(str(os.path.realpath(wpt_root))+'/'+ extension)

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

So I feel like this is basically
abs_path = os.path.realpath(os.path.join(wpt_root, path))?

extension= path+'/'
newPath=(str(os.path.realpath(wpt_root))+'/'+ extension)
# get all files from new path
paths = list(all_filesystem_paths(newPath))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

Do we first want to check if it's a directory?

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 13, 2018

Author Contributor

ah my bad, I got rid of my if statement. Putting that back in

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 13, 2018

Author Contributor

Although I am not sure if relpath was sufficient or not

paths = list(all_filesystem_paths(newPath))
# add the extension to the strings for all files from the new path,
# so they may be called from the current directory
paths=list(map(lambda path : extension + path, paths))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

A list comprehension is more idomatic here, so this would look like [extension + path for path in paths]. That said, it isn't clear what you're doing here. In the clause below the results of all_filesystem_paths are used directly, so it seems like it should be fine to do that here too.

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 13, 2018

Author Contributor

I tried that initially, but it was insufficient because all_filesystem_paths assumes we are already in the directory passed to it and will not work unless we are already operating from that directory

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 13, 2018

Build PASSED

Started: 2018-03-16 17:59:11
Finished: 2018-03-16 18:20:30

View more information about this build on:

teh-f00l added some commits Mar 13, 2018

Update lint.py
same as before except:
no functional programming
snake_case variables
checking if command line argument is a directory
Update lint.py
fixed a small variable error, and made the variable names more distinct
for path in kwargs.get("paths"):
if os.path.isdir(path):
# create extension for called directory and merge it with the existing path
new_Path = os.path.realpath(os.path.join(wpt_root, path))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

Variable names should be all lowercase.

if os.path.isdir(path):
# create extension for called directory and merge it with the existing path
new_Path = os.path.realpath(os.path.join(wpt_root, path))
extension = path+'/'

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

So this should use os.path.sep, but I'm not sure why it should be necessary.

r = os.path.realpath(wpt_root)
paths = [os.path.relpath(os.path.realpath(x), r) for x in kwargs["paths"]]
#make new list in case there are multiple added directories
paths=[]

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

Spaces between operators and operands, please.

Update lint.py
removed caps from variables, put spaces between operators and replaced 'extension' variable with os.path.sep for cross system compatibility

@gsnedders gsnedders referenced this pull request Mar 15, 2018

Closed

Update lint.py #9997

@gsnedders gsnedders changed the title Update lint.py Make lint work when directories are passed in Mar 15, 2018

@gsnedders gsnedders added the lint label Mar 15, 2018

Update lint.py
updated all_filesystem_paths to work inside the specified directory
#make new list in case there are multiple added directories
paths = []
for path in kwargs.get("paths"):
if os.path.isdir(path):

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

I think this if is missing an else clause to just add the path to the list of paths.

paths = []
for path in kwargs.get("paths"):
if os.path.isdir(path):
# create extension for called directory and merge it with the existing path

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

This indent is wrong.

@@ -59,9 +59,9 @@ def setup_logging(prefix=False):
%s: %s"""

def all_filesystem_paths(repo_root):
def all_filesystem_paths(repo_root, extended_path):

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

"extended_path" needs to be optional i.e. the signature should be

all_filesystem_paths(repo_root, subdir=None):

Also it should be relative to repo_root so that in the function we do root = os.path.join(repo_root, subdir) if subdir else repo_root or something similar.

# create extension for called directory and merge it with the existing path
new_path = os.path.realpath(os.path.join(wpt_root, path))
# get all files from new path
path_dir = list(all_filesystem_paths(wpt_root, new_Path))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

Your capitalisation is off here.

for path in kwargs.get("paths"):
if os.path.isdir(path):
# create extension for called directory and merge it with the existing path
new_path = os.path.realpath(os.path.join(wpt_root, path))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

So this assumes that path is relative to the repo_root but above it's assumed to be relative to the current working directory. I think the latter is correct.

This comment has been minimized.

Copy link
@teh-f00l

teh-f00l Mar 15, 2018

Author Contributor

The later is correct. It should have been correct here as well, new_path should be the former.

path_dir = list(all_filesystem_paths(wpt_root, new_Path))

# add all files from a given directory to our master list of files to check
paths = paths + path_dir

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 15, 2018

Contributor

I think paths.extend(path_dir) is more obvious

teh-f00l and others added some commits Mar 15, 2018

Update lint.py
updated all_filesystem_paths to work with or without specified directory arguments
Update lint.py
added an elif statement to account for files being specified in the command line 
removed excessive comments
Update lint.py
removed new_paths variable and just put the command line path input into all_filesystem_paths
Update lint.py
put spaces between '=' all operators
Update lint.py
made a mistake with my last commit where I added spaces rather than remove them
Update lint.py
text wrapper didn't work and missed some changes I needed to undo

teh-f00l added some commits Mar 16, 2018

Update lint.py
removed the space
Update lint.py
changed the elif isfile check to a simple else statement
Update lint.py
giving input files, a path relative to the repo_root
Update test_lint.py
update test_lint to mock files

@jgraham jgraham merged commit c929502 into web-platform-tests:master Mar 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@teh-f00l teh-f00l deleted the teh-f00l:patch-1 branch Mar 16, 2018

@teh-f00l teh-f00l changed the title Make lint work when directories are passed in Make lint run when directories are passed in Mar 16, 2018

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