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

Make Optional things Optional #974

Merged
merged 1 commit into from
May 23, 2023
Merged

Make Optional things Optional #974

merged 1 commit into from
May 23, 2023

Conversation

ihasdapie
Copy link
Member

@ihasdapie ihasdapie commented Jul 20, 2022 •
edited
Loading

I combed through all 154 matches of = None in the codebase and made Optional things, well, Optional

Signed-off-by: Brian Chen brian.chen@openrobotics.org

@ihasdapie ihasdapie self-assigned this Jul 20, 2022
@sloretz
Copy link
Contributor

sloretz commented Jul 20, 2022

I'Il be adding to this PR until mypy is happy and help would be appreciated :)

This is a great effort! May I suggest doing this a few files or a few classes at a time? It's easier for us maintainers to review and merge a little bit at a time than to review a lot all at once. It will also mean you have fewer merge conflicts to deal with as other PRs go in.

@fujitatomoya
Copy link
Collaborator

+1 great effort 👍

@ihasdapie ihasdapie mentioned this pull request Aug 26, 2022
Closed
@ihasdapie ihasdapie changed the title Fix type annotations Make Optional things Optional Aug 26, 2022
@ihasdapie ihasdapie marked this pull request as ready for review August 26, 2022 09:26
clalancette
clalancette requested changes Aug 31, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks generally good to me, with the exception of the failing tests. Once that is fixed, and CI is green, I'm happy to approve.

rclpy/rclpy/__init__.py Outdated Show resolved Hide resolved
@clalancette clalancette mentioned this pull request May 22, 2023
Merged
@ihasdapie @clalancette
making optional things Optional
3a64b40 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
clalancette
clalancette approved these changes May 22, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I rebased this onto the latest and fixed the remaining issues. This now looks good to me.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 31336c5 into rolling May 23, 2023
@delete-merged-branch delete-merged-branch bot deleted the brianc/typing_fixes branch May 23, 2023 02:59
@antonkesy antonkesy mentioned this pull request Oct 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz approved these changes

@clalancette clalancette clalancette approved these changes

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

@ihasdapie ihasdapie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ihasdapie @sloretz @fujitatomoya @clalancette