Skip to content

🐛 fix: allow factory method to get requested type as well as activating type#54

Closed
StummeJ wants to merge 10 commits intoNeoteroi:mainfrom
fennelmarkets:main
Closed

🐛 fix: allow factory method to get requested type as well as activating type#54
StummeJ wants to merge 10 commits intoNeoteroi:mainfrom
fennelmarkets:main

Conversation

@StummeJ
Copy link
Copy Markdown
Contributor

@StummeJ StummeJ commented Feb 27, 2024

This fixes issue #53

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.82%. Comparing base (f6d6fbc) to head (1a13acc).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #54   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files           2        2           
  Lines         565      574    +9     
=======================================
+ Hits          564      573    +9     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RobertoPrevato
Copy link
Copy Markdown
Member

RobertoPrevato commented Apr 6, 2024

Hi @StummeJ
Thank You for your contribution, and sorry for not replying earlier. I reviewed this weekend.

I tried to understand the use case for supporting this scenario, but I don't get the rationale for it. Can you please give a more realistic example?

@RobertoPrevato
Copy link
Copy Markdown
Member

I reviewed this PR again and considered again the feature request. The review from Claude Opus of the PR:

  1. Scope creep — it bundles unrelated changes: alias resolution fallbacks in _get_resolvers_for_parameters and Services.get(), flake8 config changes (E704, E701), and
    ... → pass cosmetic edits across many files.
  2. Risky behavioral change — the condition param_type is _empty was changed to param_type is _empty or param_type not in services._map. This means an annotated-but-unregistered type now silently falls through to alias-based resolution instead of raising CannotResolveParameterException. That can mask configuration errors.
  3. Breaking internal API — all three factory providers (FactoryTypeProvider, SingletonFactoryTypeProvider, ScopedFactoryTypeProvider) now pass 3 args, requiring a new
    wrapper class for existing 2-arg factories. The wrapper class also has a typo: FactoryWrapperPartentArg.
  4. The workaround is trivial — the "duplication" concern raised is solved by extracting shared logic into a helper:
def _create_engine(host, port, **opts) -> Engine: ...
    
def read_only_factory(scope, _) -> Engine:
    return _create_engine(config.reader.host, config.reader.port, readonly=True)
    
def read_write_factory(scope, _) -> Engine:
    return _create_engine(config.writer.host, config.writer.port)

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