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

lint.py: Adds a lint for files with the same basename in the same dir… #9957

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kriti21
Copy link
Contributor

commented Mar 10, 2018

This adds a lint to the all_paths_lints
set in tools/lint/lint.py that checks if
there are any paths in which 'os.path.splitext(paths)[0]`
has multiple identical values.

Closes #7570


This change is Reviewable

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

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

@kriti21

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

I have added changes according to what i understood through the comments on the issue. However, when i run ./wpt lint locally, it shows error -

Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/kriti/mozdev/web-platform-tests/tools/wpt/wpt.py", line 132, in main
    rv = script(*args, **kwargs)
  File "/home/kriti/mozdev/web-platform-tests/tools/lint/lint.py", line 794, in main
    return lint(repo_root, paths, output_format)
  File "/home/kriti/mozdev/web-platform-tests/tools/lint/lint.py", line 846, in lint
    errors = check_all_paths(repo_root, paths)
  File "/home/kriti/mozdev/web-platform-tests/tools/lint/lint.py", line 670, in check_all_paths
    errors.extend(paths_fn(repo_root, paths))
  File "/home/kriti/mozdev/web-platform-tests/tools/lint/lint.py", line 639, in check_multiple_identical_values
    if len(os.path.splitext(paths_fn)) > 0:
  File "/usr/lib/python2.7/posixpath.py", line 98, in splitext
    return genericpath._splitext(p, sep, altsep, extsep)
  File "/usr/lib/python2.7/genericpath.py", line 99, in _splitext
    sepIndex = p.rfind(sep)
AttributeError: 'function' object has no attribute 'rfind'

Please help me in correcting this. @jgraham

@w3c-bots

This comment has been minimized.

Copy link

commented Mar 10, 2018

Build BROKEN

Started: 2018-03-16 08:43:56
Finished: 2018-03-16 09:00:48

Failing Jobs

  • lint
  • tools_unittest in py27
  • tools_unittest in py36
  • tools_unittest in pypy

View more information about this build on:

@@ -624,6 +624,23 @@ def check_script_metadata(repo_root, path, f):
return errors


def check_multiple_identical_values(repo_root, path):

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

So, this function will be passed the repo_root and a list of all the paths, not a single path.

"""

errors = []
for paths_fn in all_paths_lints:

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

So here you are iterating over all the lints, not over the list of paths.


errors = []
for paths_fn in all_paths_lints:
if len(os.path.splitext(paths_fn)) > 0:

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 12, 2018

Contributor

If I assume that the input here is supposed to be a path, this is always going to be true because splitext is goingg to return a tuple of path, ext. So you need to extract the first part and keep some state to decide whether we've seen that path before. There are various ways to do that; if the list is already sorted we can just check if the current path is equal to the previous one. If we can't reply on that, the simplest approach may be to keep a dict of {path: ext}. If we encounter a path that's already in the dictionary we add it to a defaultdict(list) of {path: [exts]}, and then at the end we create an error for each entry in the latter dictionary, listing all the extensions that exist for the same path.

@kriti21 kriti21 force-pushed the kriti21:lintbugbranch branch from 901aa9a to 9678220 Mar 13, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@jgraham I have made the suggested changes. However, I don't understand what arguments to pass to error.extend() function which is showing error. Please help.

errors = []
for paths in paths:
file, file_extension = os.path.splitext(paths)
if file not in file_dict.keys():

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

There's a type in the standard library called defaultdict that encapsulates this check-and-create behaviour; let' use that.

file_dict = dict()
errors = []
for paths in paths:
file, file_extension = os.path.splitext(paths)

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

Avoid using file as a variable name since that's built in.

file_dict[file].append(file_extension)
for f in file_dict.keys():
if len(file_dict[f]) > 0:
errors.extend(paths(repo_root, paths))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 13, 2018

Contributor

So the errors list should be a list of tuples of the form (type, error, path, line). In these cases error should be some code like DUPLICATE EXTENSIONLESS PATH, error is a sitring like Got files for path %s with extensions %s, paths exclusing extensions must be unique with the %s values fileld in from the data, path can be the path without an extension, and line will be None.

@kriti21 kriti21 force-pushed the kriti21:lintbugbranch branch 2 times, most recently from 6d470f4 to d265daf Mar 16, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

@jgraham I have made the suggested changes. Please guide me how to fix the failing build.

@jgraham
Copy link
Contributor

left a comment

Sorry, this fell off the radar a bit, but it's really close now.

file_dict = dict()
errors = []
file_dict = defaultdict(list)
for paths in paths:

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 29, 2018

Contributor

for path in paths: (note no s).

file_name, file_extension = os.path.splitext(paths)
file_dict[file_name].append(file_extension)
for f in file_dict.keys():
if len(file_dict[f]) > 0:

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 29, 2018

Contributor

I think you mean > 1 here.

errors = []
file_dict = defaultdict(list)
for paths in paths:
file_name, file_extension = os.path.splitext(paths)

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 29, 2018

Contributor

path

if len(file_dict[f]) > 0:
msg = "Got files for path %s with extensions %s" % (f, file_dict[f])
errors.append(("DUPLICATE-EXTENSIONLESS-PATH", msg, f, None))

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 29, 2018

Contributor

If nothing fails you need to return an empty list return []

@kriti21 kriti21 force-pushed the kriti21:lintbugbranch branch 2 times, most recently from a5cef17 to a8a3cb0 Apr 6, 2018

lint.py: Adds a lint for files with the same basename in the same dir…
…ectory

This adds a lint to the `all_paths_lints`
set in `tools/lint/lint.py` that checks if
there are any paths in which 'os.path.splitext(paths)[0]`
has multiple identical values.

Closes #7570

@kriti21 kriti21 force-pushed the kriti21:lintbugbranch branch from a8a3cb0 to a9fb5c2 Apr 6, 2018

@kriti21

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@jgraham I have made the changes you suggested. Please have a look and guide me how to correct it so that it passes the tests.

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.