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

Create DATA_DIR constant that is pulled from env var #395

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

bendnorman
Copy link
Contributor

This PR:

  • Replaces hardcoded "/app/data" references with a constant that pulls a path from an env var defined in .env
  • Updates Google Maps API error message
  • Removes default.env because all env vars are set locally or in .env

@bendnorman bendnorman requested a review from jdangerx January 27, 2025 15:12
Copy link
Collaborator

@jdangerx jdangerx left a comment

Choose a reason for hiding this comment

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

Seems good! A couple non-blocking comments/questions but I say ship it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

non-blocking: If I wanted to change DATA_DIR or somesuch could I put that in local.env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the .env file!

@@ -91,7 +90,7 @@ def _remove_intro(
return paragraphs[idx:]
raise ValueError("Could not find starting state")

def _parse_values(self, text: str) -> None:
def _parse_values(self, text: str) -> None: # noqa: C901
Copy link
Collaborator

Choose a reason for hiding this comment

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

non-blocking: it might be worth pulling each section handler out into its own function, then using match/case or a dictionary to handle dispatch based on each section.

Are the sections always in a specific order? Or can they be a random jumble?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure tbh. Trenton did this integration way back when. I think this is something to revisit when we update the data.

@bendnorman bendnorman merged commit 802429b into init-geocodio Feb 5, 2025
1 check passed
@bendnorman bendnorman deleted the create-env-var-for-paths branch February 5, 2025 19:34
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