Skip to content

[FEAT] Add the ability to pass arguments to all packages in load_extensions method #1552

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

Closed
1 task done
rdni opened this issue Sep 10, 2023 · 3 comments
Closed
1 task done
Labels
enhancement New feature or request question Further information is requested

Comments

@rdni
Copy link
Contributor

rdni commented Sep 10, 2023

Problem Description

The load_extensions method was recently added, however it does not have the ability to pass arguments through to the setup function.

Proposed Solution

Change recursive key word into **kwargs and use kwargs.get(). Remove this then pass through the rest of the kwargs through to the setup function.

Alternatives Considered

Change *packages: str into packages: tuple[str] and pass through *args to the setup function.

Additional Information

No response

Code of Conduct

  • I agree to follow the contribution requirements.
@eightween eightween added enhancement New feature or request question Further information is requested labels Dec 9, 2023
@eightween
Copy link
Member

eightween commented Dec 9, 2023

I want to make sure I understand your request right: what exactly are you wanting a **kwargs signature to pass? We only use the recursive argument to handle pattern handling when we check your path/cwd for extension files. I don't think it's wise for us to remove it because it's important for parsing extension import resolutions, especially since we're handling numerous extensions.

If you don't mind extending off of this example, it would help me:

def load_extension(
    self,
    *packages: str,
    **kwargs
):
    for (...):
       ...
       extensions = [
          f.replace(os.path.sep, ".").replace(".py", "")
          for f in glob.glob(pattern, recursive=kwargs.get("recursive", False))
       ]

The only thing I can think of is replicating the existing logic we use for .load_extension(). To my knowledge, setup() should already allow for this as seen here.

@silasary
Copy link
Member

load_extension("filename") supports arbitrary kwargs. load_extensions("folder") does not. This issue is asking for feature parity between the two.

@eightween
Copy link
Member

I've thought about this for a while now - I'm 👎 for the feature request. These are my personal reasons for why:

  1. This is a niche use case that only satisfies lazily iterating through keyword arguments, but this can present possible issues:
    • We assume keyword arguments are passed to every extension under .load_extensions(), meaning **kwargs is forced into argument signature for extensions.
    • A developer may try to use .load_extensions() to lazily generalise passing **kwargs to select extensions. This is counterintuitive to using .load_extension(), and (if need be) iterating for extensions that actually require **kwargs.
  2. If the idea of **kwargs is for passing data like a config, you should be using env, dotenv or some way to read config values securely. Environment variables are (imo) a better solution, I don't think this is a good practice for people wanting to use the library in a simple way – this poses a lot of question.
  3. If the structure of **kwargs is a mapping that allows developers to selectively choose which extensions get it, this defeats the purpose of loading any extension in a way that's clear and easy to understand by amalgamating characteristics of both.

There are things I'm in support of:

  1. I think if the objective focus of this function is to bring feature parity, it should only:
    • be removed if we encourage using **kwargs primarily in .load_extension() – this resolves the need for a built-in iterator method.
    • keep it if we find a better solution without introducing convoluted usage.
  2. I believe we should encourage developers iterating through a list of extensions on their own. A loader() function provides more customisation, especially if paired with passing configuration data.
  3. Because the respective issue brought up a valid idea, I think .load_extension() should receive **kwargs support regardless.

I understand if these points may be debated, but I overall see no benefit in adding something (I believe to be) redundant. Passing **kwargs to .load_extension() works fine enough as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants