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.

Sign up for GitHub

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

Jump to bottom

Added Typing #311

Closed
wants to merge 17 commits into from
Closed

Added Typing #311

wants to merge 17 commits into from

Conversation

Siddhesh-Agarwal
Copy link

@huard
Copy link
Contributor

huard commented Oct 24, 2023

Thanks! I've asked a colleague to provide a review.

charlesgauthier-udm
charlesgauthier-udm reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@charlesgauthier-udm charlesgauthier-udm left a comment

Choose a reason for hiding this comment

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

Thank you for the great work @Siddhesh-Agarwal ! After review, here are my suggestions. I do not have the most experience with typing so please feel free to provide your input to the suggested changes.

xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/frontend.py Outdated Show resolved Hide resolved
xesmf/smm.py Outdated Show resolved Hide resolved
xesmf/backend.py Show resolved Hide resolved
xesmf/backend.py Outdated Show resolved Hide resolved
@Siddhesh-Agarwal
Copy link
Author

@charlesgauthier-udm I have completed the changes mentioned in the previous review. I hope these are up to your expectations.

huard
huard reviewed Nov 1, 2023
View reviewed changes
xesmf/smm.py Outdated Show resolved Hide resolved
@huard
Copy link
Contributor

huard commented Nov 1, 2023

Please resolve the test failures then this is ready to merge.

@Siddhesh-Agarwal
Copy link
Author

Siddhesh-Agarwal commented Nov 10, 2023 •
edited
Loading

@huard, it is insane to have these many pre-commit hooks. Black, Isort, and flake8 seem fine but now prettier? I am sorry but I will have to leave this PR in this state.

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@huard
Copy link
Contributor

huard commented Nov 10, 2023

Hi @Siddhesh-Agarwal

Once installed locally, pre-commit will run before each commit, so this is not something you're expected to worry about and do "by-hand".

I'm now working to address the remaining issues. I realized that by pushing to this PR, I'm pushing to your master, which might interfere with your work. Please open a PR in a separate branch to avoid this in the future.

@huard
Copy link
Contributor

huard commented Nov 10, 2023

There are a couple of failures still remaining, but I'm stumped about what might cause them.

@Siddhesh-Agarwal Siddhesh-Agarwal closed this by deleting the head repository Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesgauthier-udm charlesgauthier-udm charlesgauthier-udm left review comments

@huard huard huard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: Add Typing
3 participants
@Siddhesh-Agarwal @huard @charlesgauthier-udm