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

Dont pass settings as parameter #5125

Closed
wants to merge 1 commit into from
Closed

Dont pass settings as parameter #5125

wants to merge 1 commit into from

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Oct 18, 2024

We are passing settings around as function parameters in many places, or accessing via context.settings(). That is completely unnecessary because we can always access the static SETTINGS.

@phiresky
Copy link
Collaborator

i think maybe it makes more sense to go the other way and make the SETTINGS thing private and always access through context

one reason would be that you can't cleanly mock global state

@Nutomic
Copy link
Member Author

Nutomic commented Oct 21, 2024

Making SETTINGS private isnt possible because its defined in lemmy_utils, but context is in lemmy_api_common. So to allow access from the other crate it must be public, and then it can be used directly from anywhere in the code. Besides we dont have any tests which mock this.

@dessalines
Copy link
Member

I agree with @phiresky, we should avoid globals as much as possible, and always pass what's necessary via function params.

This is a long video about functional programming, but it really internalized for me how much better things can be when you code to avoid problems of globals and state.

@dessalines
Copy link
Member

I'm thinking we should go the other way with this: make settings (and all the other public global statics), a part of the LemmyContext initialization, to make sure they're only accessed through it.

If no one's opposed I'll make an issue and work on that.

@Nutomic
Copy link
Member Author

Nutomic commented Nov 12, 2024

@dessalines Like I said in my previous comment this is not possible because they are defined in different crates. For LemmyContext to access Settings it needs to be public, which means the entire code can access it. So it makes sense to remove it from context if that can be bypassed anyway.

@dessalines
Copy link
Member

dessalines commented Nov 12, 2024

It'd probably be better to make Settings::init public, then move the LazyLock (if we still want to keep it, its probably not necessary tho), to the LemmyContext init. Then make sure in code that Settings::init is only called once.

IMO its dangerous to be passing around this global static which requires reading files and env vars, and especially calling it from inside functions without it being explicitly passed.

@Nutomic
Copy link
Member Author

Nutomic commented Nov 13, 2024

Then we need to move LemmyContext into lemmy_utils #5200

@Nutomic Nutomic closed this Nov 13, 2024
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.

3 participants