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

Refactoring: _maybe_dereference_node #668

Merged
merged 4 commits into from
Apr 9, 2021
Merged

Refactoring: _maybe_dereference_node #668

merged 4 commits into from
Apr 9, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Apr 7, 2021 •
edited
Loading

On master, the Node._dereference_node method returns Optional[Node]. This leads to some assert node is not None statements in the code (for the benefit of mypy).

This PR does two things:

  • introduce a new method, Node._maybe_dereference_node, which returns Optional[Node]
  • change the type signature of _dereference_node to retrurn Node (not optional).

This change lets us delete some of the node is not None assertions.

This PR is a followup to #502 (comment).

@Jasha10 Jasha10 mentioned this pull request Apr 7, 2021
Merged
odelalleau
odelalleau approved these changes Apr 8, 2021
View reviewed changes
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Looking good!

omry
omry reviewed Apr 8, 2021
View reviewed changes
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

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

Looking for _dereference_node under the omegaconf module, I think there are more cases where you can eliminate the assert.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Apr 9, 2021 •
edited
Loading

I can't find any more...

omry
omry approved these changes Apr 9, 2021
View reviewed changes
Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

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

In that case shipit!

@Jasha10 Jasha10 merged commit 9d12412 into omry:master Apr 9, 2021
@Jasha10 Jasha10 deleted the reduce-mypy-asserts-for-_dereference_node branch April 9, 2021 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odelalleau odelalleau odelalleau approved these changes

@omry omry omry 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.
3 participants
@Jasha10 @omry @odelalleau