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

Type-hint comments for CarlaDataProvider #1066

Merged

Conversation

Daraan
Copy link
Contributor

@Daraan Daraan commented Mar 26, 2024

Description

This PR only adds comments that can be picked up by mypy for example and does not modify any code.

So far only type-hints for the CarlaDataProvider so that they can be picked up by IDE extensions to allow for better code completion.

TODO:

  • What is the correct type hint for _global_osc_parameters = {} # type: dict[str, Any]?
    From what I found from online resources Any fits bests, the types I've seen where [str, Number, ParameterRef]
  • Decide:
  • A) Strict: Declare variables that are None before init / after cleanup, like _world, as carla.World | None
  • B) Practical: Use their correct type without None and assume initialization and no usage after cleanup.

Note: As long as it is no main feature of carla this PR carla-simulator/carla#7009 adds type-hints for carla.


This change is Reviewable

Copy link
Contributor

@PabloVD PabloVD left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Daraan)

@Daraan Daraan force-pushed the type-hints/master-data-provider branch from 89da956 to a8b771d Compare April 12, 2024 10:57
@Daraan Daraan requested a review from PabloVD April 17, 2024 10:25
@Daraan
Copy link
Contributor Author

Daraan commented Apr 17, 2024

Sorry, didn't know that this was a request review button.😅

Any opinion on the AB choices,?

@Daraan Daraan force-pushed the type-hints/master-data-provider branch from a8b771d to c9a2084 Compare April 19, 2024 22:05
@Daraan Daraan force-pushed the type-hints/master-data-provider branch 2 times, most recently from f072d35 to c7bdac1 Compare April 30, 2024 15:57
@PabloVD
Copy link
Contributor

PabloVD commented May 3, 2024

Thanks for the PR. Given that it is a static class initialized at the beginning, we can ignore Nones. Therefore, we prefer the Practical B choice.

@PabloVD PabloVD self-assigned this May 3, 2024
@Daraan Daraan force-pushed the type-hints/master-data-provider branch from c7bdac1 to 88bb7ad Compare May 17, 2024 13:17
@Daraan
Copy link
Contributor Author

Daraan commented May 17, 2024

@PabloVD implemented the B variant by removing most None values, and increased the type-coverage

Copy link
Contributor

@PabloVD PabloVD left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Daraan)

@PabloVD PabloVD merged commit 2a23a7e into carla-simulator:master May 21, 2024
1 of 4 checks passed
@PabloVD
Copy link
Contributor

PabloVD commented May 21, 2024

@Daraan thanks for your changes, now we can approve the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants