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 methods to skip lint check and fix typehint regex #585

Merged
merged 1 commit into from May 12, 2020
Merged

Add methods to skip lint check and fix typehint regex #585

merged 1 commit into from May 12, 2020

Conversation

praveenkuttappan
Copy link
Member

Changes:

  1. Skip any modules that doesn't start with root module name (for e.g. azure) from importing
  2. Excluded known implementation methods from lint check
  3. Fix issues in typehint parsing

Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I'm a bit concerned about doing any loglevel setting outside of the init for your tool though.

logging.getLogger().setLevel() only needs to run for the entry of your app (as far as I know!)

This is just a nit though.

# load all modules and parse them recursively
for m in modules:
if not m.startswith(root_module):
Copy link
Member

Choose a reason for hiding this comment

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

Can you give me an example of this running for something we DONT own? Just for my own reference.

Copy link
Member

Choose a reason for hiding this comment

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

Good question - I'm also curious :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not fully generalized. I have tried for few external packages and it didn't work. This fix is to mainly avoid other folders in package root other than "Azure". I will debug what's wrong in generating stub for external package.

TYPEHINT_NOT_REQUIRED_METHODS = ["close",]
REGEX_ITEM_PAGED = "~[\w.]*\.([\w]*)\s?[\[\(][\w]*[\]\)]"
TYPEHINT_NOT_REQUIRED_METHODS = ["close", "__init__"]
REGEX_ITEM_PAGED = "~[\w.]*\.([\w]*)\s?[\[\(][^\n]*[\]\)]"
Copy link
Member

Choose a reason for hiding this comment

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

Making certain I grok this, you're moving to explicitly include everything up to the newline because otherwise you were just eating comments right?

Copy link
Member Author

Choose a reason for hiding this comment

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

REGEX for item paged was finding only part of return type if type is like ~azure..ItemPaged[someType[SomeSubType]]. Previous REGEX stops scan as soon as it find closing bracket.

@annatisch
Copy link
Member

@praveenkuttappan, @scbedd is correct - tools/sdks/etc shouldn't set the log level. Only add messages at an appropriate level. It would be up to the consumer of said tool to configure the log level.

You could look at moving this to the console entry point (i.e. within the actual function so as to isolate it from the lib proper) and have it configurable by one of the cmd args. Though given that it was already there I'm happy for that to be addressed in a subsequent PR

@praveenkuttappan praveenkuttappan merged commit 8e64eb7 into Azure:master May 12, 2020
@praveenkuttappan praveenkuttappan deleted the feature/whitelist_lint_excluded_methods branch May 12, 2020 16:10
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

3 participants