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

[Bug] fix for module_has_exports #50680

Closed
wants to merge 2 commits into from
Closed

Conversation

cvluca
Copy link
Contributor

@cvluca cvluca commented Jan 18, 2021

The attributes in dir(mod) may not be valid, this will throw error when calling getattr.
Use hasattr to test if it is valid.

Here is an example:

class A:
    def __init__(self, x):
        if x:
            self._attr = 1

    @property
    def val(self):
        return getattr(self, '_attr')

a = A(False)
print('val' in dir(a))
print(hasattr(a, 'val'))

b = A(True)
print('val' in dir(b))
print(hasattr(b, 'val'))

And the outputs:

True
False
True
True

@facebook-github-bot
Copy link
Contributor

Hi @cvluca!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@cvluca cvluca changed the title fix for module_has_exports [Bug] fix for module_has_exports Jan 18, 2021
@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #50680 (d9db39c) into master (1935880) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #50680      +/-   ##
==========================================
- Coverage   80.88%   80.88%   -0.01%     
==========================================
  Files        1931     1931              
  Lines      210560   210561       +1     
==========================================
- Hits       170312   170308       -4     
- Misses      40248    40253       +5     

@ngimel ngimel requested a review from eellison January 19, 2021 02:18
@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 19, 2021
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the PR! Mind adding a test so this doesn't regress in the future ?

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@cvluca cvluca requested a review from eellison January 26, 2021 11:03
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

Great, thanks for the PR!!

@eellison
Copy link
Contributor

Do you mind rebasing, looks like tests our failing due to merge conflicts ?

@cvluca cvluca force-pushed the patch-1 branch 2 times, most recently from 34811d3 to 4fd2571 Compare January 27, 2021 02:50
The attributes in `dir(mod)` may not be valid, this will throw error when calling `getattr`.
Use `hasattr` to test if it is valid.
@cvluca
Copy link
Contributor Author

cvluca commented Jan 27, 2021

Fixed, all green now!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@eellison merged this pull request in 3f23ad5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants