Skip to content

[FBcode->GH] Add import to fix linter error #5013

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

Closed

Conversation

prabhat00155
Copy link
Contributor

#4973 introduced a linter error internally(D32694312):

Linter (FLAKE8)raised a F821 error online 236-
undefined name 'requests'

@facebook-github-bot
Copy link

facebook-github-bot commented Nov 30, 2021

💊 CI failures summary and remediations

As of commit de25166 (more details on the Dr. CI page):


None of the CI failures appear to be your fault 💚



🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@prabhat00155
Copy link
Contributor Author

prabhat00155 commented Nov 30, 2021

The import fails in our tests. Should I remove the type annotation? cc @pmeier

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

I know this a FBcode=>GH sync and that eventually we will have to merge, just blocking to ensure we discuss it a bit.

I think the right way to address this would be to delete _get_confirm_token() method which seems not being used. The requests package was recently removed and I bet we missed clearing this method.

@pmeier could you please confirm?

If that's the case then we can merge the PR to sync internal/external and the follow up with one that removes the import and the private method completely.

@prabhat00155
Copy link
Contributor Author

prabhat00155 commented Nov 30, 2021

I think the right way to address this would be to delete _get_confirm_token() method which seems not being used. The requests package was recently removed and I bet we missed clearing this method.

Yeah, I don't see _get_confirm_token() getting used anywhere; could well be removed.

@pmeier
Copy link
Collaborator

pmeier commented Nov 30, 2021

I think the right way to address this would be to delete _get_confirm_token() method which seems not being used. The requests package was recently removed and I bet we missed clearing this method.

Exactly. I'll send a PR.

@prabhat00155
Copy link
Contributor Author

prabhat00155 commented Nov 30, 2021

I think the right way to address this would be to delete _get_confirm_token() method which seems not being used. The requests package was recently removed and I bet we missed clearing this method.

Exactly. I'll send a PR.

@pmeier Don't send a PR. I'll update this PR after making the changes internally. This will simplify the internal sync.

@datumbox
Copy link
Contributor

@prabhat00155 Updating this PR means we will need to export it via the script to sync it again. Because this is tagged as FBcode->GH it will be automatically excluded. So usually what we do (because it's the easiest way) is merge the original PR as-is and then fix the issue on a follow up PR which can easily be synced to FBcode. Would you be OK doing this?

@prabhat00155
Copy link
Contributor Author

@prabhat00155 Updating this PR means we will need to export it via the script to sync it again. Because this is tagged as FBcode->GH it will be automatically excluded. So usually what we do (because it's the easiest way) is merge the original PR as-is and then fix the issue on a follow up PR which can easily be synced to FBcode. Would you be OK doing this?

I have already made this change in the diff to fix the error. So, we won't need to sync this change again, hence the FBcode->GH tag.

@prabhat00155 prabhat00155 deleted the prabhat00155/fix_linter_error branch November 30, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants