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

Centralize the configuration handling #503

Open
Szasza opened this issue Apr 1, 2024 · 12 comments
Open

Centralize the configuration handling #503

Szasza opened this issue Apr 1, 2024 · 12 comments

Comments

@Szasza
Copy link
Contributor

Szasza commented Apr 1, 2024

As parsedmarc's functionality gets richer, the functions' parameter lists are getting longer due to the config being parsed on top level and all the values are individually passed down in the call chain. This will yield enormous parameter lists over time.

It would be great to extract the configuration into a standalone class, so it could be included in any depth and accessed in a more lightweight manner, reducing the amount of passed parameters.

@seanthegeek
Copy link
Contributor

I got excited when I saw this subject thinking this was a PR. 😝

This is absolutely needed. It's a mess already! I'm just not sure what is the right way to redesign it. So many people have contributed code over the years that most of the code isn't mine anymore. It's a pain for me to get around the code, let alone someone brand new to the project.

@seanthegeek
Copy link
Contributor

I just reread your post. That design could work!

@seanthegeek seanthegeek pinned this issue Apr 1, 2024
@Szasza
Copy link
Contributor Author

Szasza commented Apr 1, 2024

@seanthegeek I will work on the implementation then.

@seanthegeek
Copy link
Contributor

seanthegeek commented Apr 2, 2024

Thinking about it more, having all options as a config object would break things for people who are using specific functions of parsedmarc as a library. How about a config dictionary instead that could be passed to existing functions as **kwargs?

@Szasza
Copy link
Contributor Author

Szasza commented Apr 2, 2024

@seanthegeek it can be done, however it will make the functions slightly more complex. I think it may be a better idea to think about which parts/functions of parsedmarc are used as standalone.

Is parsedmarc meant to be a self-contained app or a utility library?

@seanthegeek
Copy link
Contributor

Ideally it should be both, with some core functions used for parsing reports usable as a library, with some parts on top of that like mailbox monitoring being standalone. Separating those into _ prefixed modules is going to take some time. I'll give it more thought when I have some time this weekend.

@rodpayne
Copy link
Contributor

With the configuration in a standalone class whose instance is passed to all functions, how about including the global dictionaries and caches (such as IP_ADDRESS_CACHE, REVERSE_DNS_MAP, etc.) so that every function can access them with only the one reference parameter being passed down the chain? For example, if every function had an argument of parsedmarc_globals, the function could use parsedmarc_globals.opts.dns_timeout, parsedmarc_globals.ip_address_cache, etc., as needed)

@Szasza
Copy link
Contributor Author

Szasza commented May 10, 2024

@seanthegeek I think @rodpayne's proposal is close to what you would like to see.

@lingfish
Copy link
Contributor

Would something like dynaconf help here?

@Szasza
Copy link
Contributor Author

Szasza commented Jun 27, 2024

@lingfish I would think so. @seanthegeek thoughts on https://www.dynaconf.com/?

@seanthegeek
Copy link
Contributor

Just new getting around to looking at this. Dynaconf looks really nice!

@Szasza
Copy link
Contributor Author

Szasza commented Sep 26, 2024

@seanthegeek if you are happy with the dynaconf approach, I can give the implementation a shot in the next 2 weeks.

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

No branches or pull requests

4 participants