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

Support extraction by filename as well as directory #324

Merged
merged 1 commit into from Jan 15, 2016

Conversation

Projects
None yet
5 participants
@ENuge
Contributor

ENuge commented Jan 12, 2016

Reason for the change: #253 . This obviates the need for parallelizing to make things faster (on the consumer's side, we only scan files that are different in a given branch compared to origin's master, which brings scan time down from ~4min 30s to ~5-20s, depending on how old one's branch is).

Code changes: moved half of the logic from extract_from_dir into its own function. Then added a few "is this a path or directory?" type checks to frontend.py to use the right thing. Those checks are kind of ugly right now but I couldn't think of a more elegant way of doing it. Added/rejiggered a couple of tests.

@gitmate-bot gitmate-bot added the size/L label Jan 12, 2016

@sils

This comment has been minimized.

Member

sils commented Jan 12, 2016

Hi, can you fix the issues pointed out by the gitmate bot and refactor your stuff into atomic commits as described in http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/ ?

@sils

This comment has been minimized.

Member

sils commented Jan 12, 2016

Thanks for submitting :)

@codecov-io

This comment has been minimized.

codecov-io commented Jan 12, 2016

Current coverage is 88.38%

Merging #324 into master will increase coverage by +0.04% as of 59793db

@@            master    #324   diff @@
======================================
  Files           23      23       
  Stmts         3612    3626    +14
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3191    3205    +14
  Partial          0       0       
  Missed         421     421       

Review entire Coverage Diff as of 59793db

Powered by Codecov. Updated on successful CI builds.

@ENuge

This comment has been minimized.

Contributor

ENuge commented Jan 12, 2016

Appeased the LineLengthBot with Yelp's preferred syntax (which is vertically long, but eh this line is super-awkward regardless).

I'll refactor my commits later today or tomorrow. (I started this change on Yelp's old fork of babel, so this version was more of a copy/paste job. But I'll make the history make some sense.)

@sils

This comment has been minimized.

Member

sils commented Jan 12, 2016

@ENuge cool, ping us when it's done.

strip_comment_tags=self.strip_comments
)
else:
extracted = check_and_call_extract_file(path, method_map,

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Could you use the same indent style as in the extract_from_dir call above? :)

This comment has been minimized.

@ENuge

ENuge Jan 13, 2016

Contributor

Yup! Done.

if os.path.isdir(path):
filepath = os.path.normpath(os.path.join(path, filename))
else:
filepath = filename

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Add a comment like # already normalized

options = odict
if callback:
callback(filename, method, options)
for lineno, message, comments, context in \

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Maybe don't extract the tuple here?

for message_tuple in extract...:
   yield (filename,) + message_tuple
filename = relpath(filepath, dirpath)
for pattern, method in method_map:
if pathmatch(pattern, filename):

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Would be better to invert this if (avoids over-indentation):

if not pathmatch(pattern, filename):
  continue
options = ....
:param dirpath: the path to the directory to extract messages from.
"""
if dirpath is None:
dirpath = os.getcwd()

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

This should not be done in this function; it's fine to just do it in the caller.

EDIT: To expand on what I mean, this function is too low-level for it to be reading facts from the execution environment (the current working directory in this case); it's better to have the callers explicitly pass this in.

This comment has been minimized.

@ENuge

ENuge Jan 13, 2016

Contributor

fwiw extract_from_dir(..) above it does the same check. Though I'm fine with having the caller explicitly pass it in.

and include in the results
:param strip_comment_tags: a flag that if set to `True` causes all comment
tags to be removed from the collected comments.
:param dirpath: the path to the directory to extract messages from.

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Can you expand a little on this -- namely that this is used to calculate the relative names of files?

for path in self.input_paths:
if not os.path.isdir(path) and not os.path.isfile(path):
raise DistutilsOptionError("Input path: %s is not a file or directory" % path)

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Is this even possible? :D

This comment has been minimized.

@ENuge

ENuge Jan 13, 2016

Contributor

Heh, this is meant to be checking if the path exists. I didn't really think about the much more direct way of doing that...namely, using os.path.exists(..). Will update.

break
filepath = os.path.join(root, filename).replace(os.sep, '/')
for (

This comment has been minimized.

@akx

akx Jan 12, 2016

Member

Don't expand the tuple here either.

for message_tuple in check...:
    yield message_tuple

is fine and dandy!

This comment has been minimized.

@ENuge

ENuge Jan 13, 2016

Contributor

Cool, I'll add a comment/docstring line with the return type or something to make up for the loss in explicitness.

EDIT: The docstring already does that! \o/

@akx

This comment has been minimized.

Member

akx commented Jan 12, 2016

And aside from the handful of comments I had, great work @ENuge ! :)

I'd appreciate a test that passes in a direct filename to the command line command though. :)

EDIT: Wagh, I'm a dummy who can't scroll down.

@akx

This comment has been minimized.

Member

akx commented Jan 13, 2016

@ENuge: Looking good!

Please squash all of the commits into one though :)

@sils

This comment has been minimized.

Member

sils commented Jan 13, 2016

Interesting, travis errored out on mac only for pypy 2.6, maybe a nondeterministic bug?

@akx

This comment has been minimized.

Member

akx commented Jan 13, 2016

@sils1297 Heh, yes... race condition against the clock, it seems. If the wallclock ticks forward a second while those tests are being run, they may fail.

We should probably freeze time for the duration of those tests or maybe compare the outputs ignoring datetime differences.

@ENuge ENuge force-pushed the ENuge:eoin_babel_extract_with_files branch from 79ddbf4 to 4b0d4c5 Jan 14, 2016

Eoin Nugent
extraction: Babel now supports extraction by filename as well as by dir
One can now supply a filename or a directory to be extracted. For
large codebases, this allows the consumer to optimize their
string extraction process by, for instance, only supplying the
files that have actually been changed on the given dev's branch
compared to master.

Relates to #253 . I
don't want to say "fixes", but makes further optimization
unnecessary for most use cases.

@ENuge ENuge force-pushed the ENuge:eoin_babel_extract_with_files branch from 4b0d4c5 to 19957e2 Jan 14, 2016

@akx

This comment has been minimized.

Member

akx commented Jan 15, 2016

Super nice work @ENuge!

I'm gonna merge this, but come to think of it, we should add an alias for input-paths called input-dirs for backward compatibility. I'll make an issue out of that. :)

akx added a commit that referenced this pull request Jan 15, 2016

Merge pull request #324 from ENuge/eoin_babel_extract_with_files
Support extraction by filename as well as directory

@akx akx merged commit 459d30f into python-babel:master Jan 15, 2016

6 of 7 checks passed

review/gitmate/manual This commit needs review.
Details
Scrutinizer 7 updated code elements
Details
codecov/patch 100.00% of diff hit (target 80.00%)
Details
codecov/project 88.38% (+0.04%) compared to a4cd0e3 at 88.34%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit All is well! :)
Details

@pyup-bot pyup-bot referenced this pull request Jan 6, 2017

Merged

Update babel to 2.3.4 #13

@pyup-bot pyup-bot referenced this pull request Jan 31, 2017

Open

Update babel to 2.3.4 #28

@pyup-bot pyup-bot referenced this pull request Apr 11, 2017

Open

Initial Update #3

@pyup-bot pyup-bot referenced this pull request May 12, 2017

Closed

Initial Update #43

@pyup-bot pyup-bot referenced this pull request Jul 4, 2017

Merged

Initial Update #2

@pyup-bot pyup-bot referenced this pull request Nov 3, 2017

Closed

Update babel to 2.5.1 #424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment