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

Better imports #230

Closed
shtlrs opened this issue Jan 10, 2024 · 3 comments
Closed

Better imports #230

shtlrs opened this issue Jan 10, 2024 · 3 comments
Labels

Comments

@shtlrs
Copy link
Contributor

shtlrs commented Jan 10, 2024

I have seen a pattern where certain modules are imported from within functions.

This is usually used as a hack to avoid circular imports, which brings the question of why we have them like that ?

This pattern has caused the e2e tests to fail in #229 ( see all of my comments)

The erros is mostly do to unusual binding of values inside modules/ to variables, which can lead a variable to have 2 states at runtime, whereas we only want it to have one across the entire app's lifecycle.

@shtlrs shtlrs changed the title Load modules once, and better. Better imports Jan 11, 2024
@supakeen
Copy link
Owner

So, this is related to #232. Those imports aren't there to prevent circular imports but they are there to make sure those modules only get imported after the configuration is setup.

I'm unsure if we want to close this as a duplicate or track separately.

@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 11, 2024

So, this is related to #232. Those imports aren't there to prevent circular imports but they are there to make sure those modules only get imported after the configuration is setup.

I'm unsure if we want to close this as a duplicate or track separately.

How keen are you on keeping the configuration as simple variables like this ?

I was thinking about using pydantic models, for the following reasons:

  1. We'd have a better structure for configs.
  2. It's easier to patch class instances, or even replace them.
  3. Pydantic comes with a native support of loading from the environment AND from .env files.
  4. Something that can be used in Override configuration through CLI #232: Defining options as some sort of objects, that we'd load from these model classes. The advantage is that we get to access both the variable names and its metadata: types, etc. That we can use later to inject into click types / converters.

@supakeen supakeen changed the title Better imports Better configuration system Jan 12, 2024
@supakeen supakeen changed the title Better configuration system Better imports Jan 12, 2024
@shtlrs
Copy link
Contributor Author

shtlrs commented Mar 8, 2024

This has been fixed in #239

@shtlrs shtlrs closed this as completed Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants