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

chore: cleanup unneeded references to typing module #63

Merged
merged 5 commits into from
May 24, 2024

Conversation

mackenzie-grimes-noaa
Copy link
Contributor

Linear Issue

IDSSE-539

Changes

No logic changes, just removing typing module imports wherever possible and using primitive types (like dict and tuple) for type annotations instead.

Explanation

N/A

Copy link
Contributor

@Geary-Layne Geary-Layne left a comment

Choose a reason for hiding this comment

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

Looks good, I found a few more changes, and wonder if you changed all the doc strings


logger = logging.getLogger(__name__)


class Config:
"""Configuration data class"""
def __init__(self,
config: Union[dict, List[dict], str],
keys: Optional[Union[list, str]] = None,
config: dict | Iterable[dict] | str,
Copy link
Contributor

Choose a reason for hiding this comment

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

I normally use Sequence but Iterable is likely more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I read somewhere that Iterable was preferred for arguments because it gives the most flexibility to the caller, whereas Sequence was preferred for return values because it's more specific, and you (hopefully) know what exactly you're returning.

In general I too prefer Sequence.

python/idsse_common/idsse/common/json_message.py Outdated Show resolved Hide resolved

from pika import BasicProperties, ConnectionParameters, PlainCredentials
from pika.adapters import BlockingConnection, blocking_connection
from pika.adapters import BlockingConnection
from pika.adapters.blocking_connection import BlockingChannel
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting this explicitly make the code easier to read.

width: float,
height: float,
dx: float,
dy: Optional[float] = None):
dy: float | None = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

In a previous file you used dy: float, optional = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I think the "float, optional" has showed up when I allowed VSCode to auto-generate the docstring, vs. "float | None" is what I typed manually. I'll change this to use "float, optional"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, figured it out. "optional" means a default value is defined for the argument, vs. "float | None" just means the value can be None. I'll fix these type hints now.

python/idsse_common/test/test_publish_confirm.py Outdated Show resolved Hide resolved
mackenzie-grimes-noaa and others added 3 commits May 24, 2024 09:38
Co-authored-by: Geary-Layne <77741618+Geary-Layne@users.noreply.github.com>
Co-authored-by: Geary-Layne <77741618+Geary-Layne@users.noreply.github.com>
Copy link
Contributor

@Geary-Layne Geary-Layne left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -28,21 +28,18 @@

def set_corr_id_context_var(
originator: str,
key: Optional[uuid.UUID] = None,
issue_dt: Optional[Union[str, datetime]] = None
key: uuid.UUID = uuid.uuid4(),
Copy link
Contributor

Choose a reason for hiding this comment

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

uuid4() is new to me, I'm guessing that is what we should be using everywhere, I'll try to remember this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied code from inside this function, which I think was using uuid4(). I've seen places in our code that alias uuid4() as just uuid(), because it's the most common version people think of when they say "UUID". The format is xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx.

There are other versions of the UUID standard, such as version 1 which uses the current time and the MAC address of the computer to generate a probably unique ID, but I've only ever used v4.

https://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_(random)

@mackenzie-grimes-noaa mackenzie-grimes-noaa merged commit 583c23c into main May 24, 2024
2 checks passed
@mackenzie-grimes-noaa mackenzie-grimes-noaa deleted the chore/remove-typing-imports branch May 24, 2024 18:21
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