Skip to content

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.

@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.

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
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