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 support for parsing dotted names into tags #57

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kaushikcfd
Copy link
Collaborator

No description provided.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Some initial high-level feedback below.

pytools/tag.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
test/testlib.py Outdated Show resolved Hide resolved
@kaushikcfd kaushikcfd force-pushed the parse_tag branch 2 times, most recently from 44f0ead to c2f9057 Compare January 4, 2021 20:34
- limit line columns length to 85
- install lark-parser for pylint,docs to resolve modules
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks! Some initial comments below.

pytools/tag.py Outdated Show resolved Hide resolved
pytools/tag.py Outdated Show resolved Hide resolved
pytools/tag.py Outdated Show resolved Hide resolved
@@ -7,7 +7,7 @@ Pytest:
# AK, 2020-12-13
rm pytools/log.py

export EXTRA_INSTALL="numpy"
export EXTRA_INSTALL="numpy lark-parser"
Copy link
Owner

Choose a reason for hiding this comment

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

Could you push a sister MR to Gitlab (add the link to the PR description) to ensure that passes, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pytools/tag.py Outdated Show resolved Hide resolved
pytools/tag.py Outdated
Comment on lines 268 to 304
TAG_GRAMMAR = """
tag: tag_class "(" params ")" -> map_tag_from_python_class
| SHORTCUT -> map_tag_from_shortcut

params: -> map_empty_args_params
| args -> map_args_only_params
| kwargs -> map_kwargs_only_params
| args "," kwargs -> map_args_kwargs_params

?kwargs: kwarg
| kwargs "," kwarg -> map_kwargs

args: arg -> map_singleton_args
| args "," arg -> map_args

kwarg: name "=" arg -> map_kwarg

?arg: tag
| INT -> map_int
| ESCAPED_STRING -> map_string

tag_class: module "." name -> map_tag_class

module: name -> map_top_level_module
| module "." name -> map_nested_module

name: CNAME -> map_name
SHORTCUT: "." ("_"|LETTER) ("_"|LETTER|DIGIT|".")*

%import common.INT
%import common.ESCAPED_STRING
%import common.DIGIT
%import common.LETTER
%import common.CNAME
%import common.WS
%ignore WS
"""
Copy link
Owner

Choose a reason for hiding this comment

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

Looked over the grammar, looks OK. (Please leave this for me to resolve at final review.)

@kaushikcfd kaushikcfd mentioned this pull request Mar 8, 2021
3 tasks
Base automatically changed from master to main March 8, 2021 01:59
@kaushikcfd
Copy link
Collaborator Author

kaushikcfd commented Mar 11, 2021

Probably I should rewrite this as pytools.resolve_name already contains most of the logic.

@inducer
Copy link
Owner

inducer commented Mar 12, 2021

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@inducer
Copy link
Owner

inducer commented Mar 15, 2021

Probably I should rewrite this as pytools.resolve_name already contains most of the logic.

I actually don't think so. We'll be able to reuse this as we use more lark. Sorry I've been slow to review. Otherwise ready for another look from your end?

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.

2 participants