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

Allow more configuration of the besticon package #75

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

koesie10
Copy link
Contributor

This adds a new Besticon struct from which all the other methods
are called. This allows removing all global variables and constants
and replacing them by functional options. This makes it easier to
configure all options with e.g. different named environment variables.

This is a breaking change since it changes how all methods need to be
called.

Something which is not in this PR but may be useful is to remove the
IconFinder and change it to be a FetchIcons method on the Besticon
struct which takes functional options as well.

See: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

This adds a new Besticon struct from which all the other methods
are called. This allows removing all global variables and constants
and replacing them by functional options. This makes it easier to
configure all options with e.g. different named environment variables.

This is a breaking change since it changes how all methods need to be
called.

See: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
@koesie10
Copy link
Contributor Author

@mat I'm not sure why CodeQL is failing since there's no new code causing these violations. Are the previous ones perhaps not detected or ignored?

Copy link
Owner

@mat mat left a comment

Choose a reason for hiding this comment

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

Nice refactoring, thank you 👍

@mat
Copy link
Owner

mat commented Jan 24, 2022

@mat I'm not sure why CodeQL is failing since there's no new code causing these violations. Are the previous ones perhaps not detected or ignored?

Not detected I'm afraid :-( Let's deal with them separately later…

@mat mat merged commit 4891fda into mat:master Feb 7, 2022
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