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

Remove unused module + enhance code patterns #849

Merged
merged 7 commits into from
Dec 5, 2021

Conversation

alaabenfatma
Copy link
Contributor

This PR targets this issue: #848

What's new:

  • Import modules with respect to alphabetical order
  • Remove unused module MetaflowException
  • Elegantly check the Python version to know what modules to import
    • Use a more explicit LBYL instead of EAPF

@savingoyal
Copy link
Collaborator

@alaabenfatma Thanks for the PR! The codebase currently doesn't advocate for either of EAPF or LBYL, so I am not sure if modifying the version check would achieve any goal.
Removing the unused module is a very welcome change. If you can decouple it from the LBYL change, we can get this PR merged.

@alaabenfatma
Copy link
Contributor Author

alaabenfatma commented Dec 4, 2021

@alaabenfatma Thanks for the PR! The codebase currently doesn't advocate for either of EAPF or LBYL, so I am not sure if modifying the version check would achieve any goal. Removing the unused module is a very welcome change. If you can decouple it from the LBYL change, we can get this PR merged.

@savingoyal Thanks for the review. I decoupled the LBYL part from the PR as requested.
I suggested the use of LBYL in order to make the code more consistent with the rest of the codebase.
The rest of the codebase uses LYBL instead of EAPF:
Examples:

@savingoyal
Copy link
Collaborator

@alaabenfatma Thanks for the PR! The codebase currently doesn't advocate for either of EAPF or LBYL, so I am not sure if modifying the version check would achieve any goal. Removing the unused module is a very welcome change. If you can decouple it from the LBYL change, we can get this PR merged.

@savingoyal Thanks for the review. I decoupled the LBYL part from the PR as requested. I suggested the use of LBYL in order to make the code more consistent with the rest of the codebase. The rest of the codebase uses LYBL instead of EAPF: Examples:

Thanks for the edit! Re: EAPF vs LBYL - unfortunately there isn't any consistency in the code base today - EAPF example - https://github.com/Netflix/metaflow/blob/master/metaflow/util.py#L14

@savingoyal savingoyal merged commit e72712a into Netflix:master Dec 5, 2021
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.

2 participants