nayaverdier / dyntastic

A DynamoDB library on top of Pydantic and boto3.
MIT License
55 stars 12 forks source link

added the ability to set and using environment variables. Added exa… #17

Closed peterb154 closed 8 months ago

peterb154 commented 8 months ago

…mple of dynamic in readme fixed #15

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (b58c010) 100.00% compared to head (caed4ab) 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #17 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 6 Lines 656 669 +13 Branches 123 127 +4 ========================================= + Hits 656 669 +13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nayaverdier commented 8 months ago

@peterb154 Thanks for the PR! I think it might be preferred to only lookup the environment variables if the value is not specified on the class, to avoid unexpected behavior during development. Setting a value on code seems "stronger" to me than setting an environment variables

In most cases, either the production version will define the environment variables instead of hard coding, or even more common is not needing to specify the host, so the field is never set and setting the env var locally will override it as desired.

I'll add in support to set those with a callable as well before releasing, to make it more flexible for more complex usages.

Also, it might be useful to add a test to ensure the priority of the field vs environment variables (for whichever priority we land on).

peterb154 commented 8 months ago

@peterb154 Thanks for the PR! I think it might be preferred to only lookup the environment variables if the value is not specified on the class, to avoid unexpected behavior during development. Setting a value on code seems "stronger" to me than setting an environment variables

Obviously, this is your project, and giving preference to hard coded __table_host__ and __table_region__ makes sense I guess, especially when a dev will probably use one of the two, not both.

In most cases, either the production version will define the environment variables instead of hard coding, or even more common is not needing to specify the host, so the field is never set and setting the env var locally will override it as desired.

In production, I would say that typically no meta or env vars for table and region will be defined, as users will apps be using the default host and region setter provided in the aws client/resource/session etc.

I'll add in support to set those with a callable as well before releasing, to make it more flexible for more complex usages.

Also, it might be useful to add a test to ensure the priority of the field vs environment variables (for whichever priority we land on).

Sure. If you want me to re-work the pr, I can do that. Or you can fine tune it to your preferences from here. No problem either way.

nayaverdier commented 8 months ago

Feel free to make the changes if you want, I'm out of town and won't have access to my computer until later in the week. Otherwise I'll make the tweaks when I'm back, either way.

peterb154 commented 8 months ago

@nayaverdier I just updated the PR to

  1. Prioritize __table__host__ over env var DYNATISTIC_HOST with tests to prove working
  2. Prioritize __table__region__ over env var DYNATISTIC_REGION with tests to prove working
  3. Allow __table__host__ & __table__region__ to be callables with tests to prove working
  4. updated Readme to reflect these changes
nayaverdier commented 8 months ago

Looks great, thanks for the changes! I just did a small refactor to isolate the resolution of the fields vs assignment. Will merge and deploy shortly.