ryansurf / cli-surf

Get surf and ocean data from the command line interface
7 stars 10 forks source link

Refactor Use `pydantic-settings` for env vars #28

Closed K-dash closed 1 month ago

K-dash commented 1 month ago

@ryansurf Sorry for bombarding you with pull requests one after another!

I refactored the environment variable loading process (os.getenv()) and used pydantic-settings to make it type-safe!

Maybe you know, The benefits of pydantic-settings are:

Thanks to this, send_email.py now throws an error if the email address isn't set, for example.

This changed commit: https://github.com/ryansurf/cli-surf/pull/28/commits/c2f5bf51c8545c8ac5ae3ff51be9c55b16d725e0

I'd be stoked if you could merge this in!!


Also, I know this article is in Japanese, but I wanted to share a piece I wrote about pydantic-settings:) https://qiita.com/inetcpl/items/b4146b9e8e1adad239d8

ryansurf commented 1 month ago

This is awesome. I didn't know about pydantic-settings but it looks great.

For send_email.py, will an error be thrown but everything runs as expected? I think that's what will happen, the email settings are optional

Nice job on this PR! Article was interesting too :D

Merging

ryansurf commented 1 month ago

Just to make sure, we can get rid of the .env file, right? Redoing the docs

K-dash commented 1 month ago

@ryansurf Thanks for merging this PR! You're right, the email settings are optional, and when send_email.py runs, it will check if the values in .env conform to the expected types. As long as send_email.py isn't executed, other processes like server.py and cli.py won't be affected. I'm planning to add some test code later to verify that the settings in settings.py are correct.

Also, thanks for checking out my article! :D

K-dash commented 1 month ago

Just to make sure, we can get rid of the .env file, right? Redoing the docs

Thanks for the follow-up question. If I understand correctly, you're asking about the behavior when running the code without the .env file (i.e., when the .env file doesn't exist), right?

ryansurf commented 1 month ago

Exactly, from my understanding the settings.py file replaces the .env file. Is that correct?

K-dash commented 1 month ago

Exactly, from my understanding the settings.py file replaces the .env file. Is that correct?

No, that's not quite right! The settings.py file is responsible for loading and validating the types of values from the .env file. This means that the .env file is still necessary, and users will need to continue writing configuration values in the .env file, just like before.

K-dash commented 1 month ago
# settings.py
class CommonSettings(BaseSettings):
    """
    Base class for defining common settings.
    model_config (SettingsConfigDict)Configuration dictionary
    for specifying the settings file and encoding.
    """

    model_config = SettingsConfigDict(
        env_file=f"{Path(__file__).parent.parent}/.env",
        env_file_encoding="utf-8",
        extra="ignore",
    )

The .env file is being loaded by the settings.py file through the line env_file=f"{Path(__file__).parent.parent}/.env".

K-dash commented 1 month ago

Apologies for the lack of clarity in my explanation!

To summarize:

Sorry for the confusion😵‍💫

K-dash commented 1 month ago

Please point out if I'm missing the point of your response, as I may not have understood the exact intent of your question.🙂‍↕️

ryansurf commented 1 month ago

Ah, that makes sense. Sorry, I was being a bit lame, you answered what I was asking :thumbsup: