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

Allow wildcards inside of configuration section names #5120

Merged
merged 7 commits into from May 30, 2018
Merged

Conversation

msullivan
Copy link
Collaborator

This is to support Django-style usecases with patterns like
site.*.migrations.*.

The implementation works by mixing in the old-style glob matching with
the new structured matching.

Fixes #5014.

This is to support Django-style usecases with patterns like
`site.*.migrations.*`.

The implementation works by mixing in the old-style glob matching with
the new structured matching.

Fixes #5014.
@msullivan msullivan requested a review from gvanrossum May 29, 2018 22:19
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nots. More later.

@@ -1179,10 +1179,37 @@ warn_unused_configs = True
[[mypy-spam.eggs]
[[mypy-emarg.*]
[[mypy-emarg.hatch]
[[mypy-a.*.b]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't a.*.b considered unused?

Patterns may also be "unstructured" wildcards, in which ``*``s may
appear in the middle of a name (e.g
``site.*.migrations.*``). Internal ``*``s match one or more module
component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

componentS. Also I'd write "stars" instead of "*s" (both places).

mypy/options.py Outdated
for glob in unstructured_glob_keys:
self.glob_options.append((glob, re.compile(fnmatch.translate(glob))))

# We (for ease of implementation), treat unstructured glob
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No comma.

mypy/options.py Outdated

# We (for ease of implementation), treat unstructured glob
# sections as used if any real modules use them or if any
# concrete config sections use them. This means we need to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a little shady, and explain why a.*.b is not an error in the test, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll add a comment to the test.

mypy/options.py Outdated
self.per_module_cache[key] = new_options

self.unused_configs = set(keys)
# Add the more structured sections into unused configs .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before period

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more requests for clarification of comments, and I think there's an issue with whether foo.bar matches foo.*.bar.

mypy/options.py Outdated
# 2. "Unstructured" glob patterns: foo.*.baz, in the order they appear in the file
# 3. "Well-structured" wildcard patterns: foo.bar.*, in specificity order.

# Since structured configs inherit from glob configs above them in the hierarchy,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't explain "glob config" -- presumably this is the same as structured config? I.e. this implements the "in specificity order" of point 3 above.

mypy/options.py Outdated
# Config precedence is as follows:
# 1. Concrete section names: foo.bar.baz
# 2. "Unstructured" glob patterns: foo.*.baz, in the order they appear in the file
# 3. "Well-structured" wildcard patterns: foo.bar.*, in specificity order.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit head-exploding, since conflict resolution for (2) is based on file order while for (3) it is based on pattern length.

Plus I don't have a good intuition whether "file order" means "first in file wins" or "last in file wins".

# To do this, process all glob configs before non-glob configs and
# We have to process foo.* before foo.bar.* before foo.bar,
# and we need to apply *.bar to foo.bar but not to foo.bar.*.
# To do this, process all well-structured glob configs before non-glob configs and
# exploit the fact that foo.* sorts earlier ASCIIbetically (unicodebetically?)
# than foo.bar.*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add (redundantly?) that "processed last" translates into "wins"?

mypy/options.py Outdated
break

# OK and *now* we need to look for glob matches
if not module.endswith('.*'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would module ever end in a star?

mypy/options.py Outdated
new_options = Options()
new_options.__dict__.update(options.__dict__)
new_options.__dict__.update(self.per_module_options[key])
new_options = options.apply_changes(self.per_module_options[key])
self.per_module_cache[key] = new_options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps combine with previous line? (Unless it doesn't fit.)

mypy/options.py Outdated
concrete = [k for k in structured_keys if not k.endswith('.*')]

for glob in unstructured_glob_keys:
self.glob_options.append((glob, re.compile(fnmatch.translate(glob))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this means that module foo.bar does not match the pattern foo.*.bar. And that's surprising since foo.bar does matchs foo.bar.*.

@gvanrossum
Copy link
Member

gvanrossum commented May 30, 2018 via email

mypy/options.py Outdated
# match *zero or more* module sections. This means we compile
# '.*' into '(\..*)?'. We also need to escape .s in the glob, so
# we hackily rewrite .s to ,s to accomplish this.
s = s.replace('.', ',').replace(',*', '(\..*)?').replace(',', '\.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work right for *.foo I think. Also maybe the code would be more readable if you just started with module.split('.') and applied re.escape() to parts that aren't *, and something else for other parts, special-casing a * at the front.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

@msullivan msullivan merged commit c29210e into master May 30, 2018
@msullivan msullivan deleted the globtion branch May 30, 2018 20:17
msullivan added a commit that referenced this pull request May 30, 2018
This is to support Django-style usecases with patterns like
`site.*.migrations.*`.

The implementation works by mixing in the old-style glob matching with
the new structured matching.

Fixes #5014.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants