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

fix: Ensure app-dir is appended to path during autodiscovery. #2277

Merged
merged 8 commits into from Sep 6, 2023

Conversation

sykloid
Copy link
Contributor

@sykloid sykloid commented Sep 3, 2023

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

  • This PR fixes a bug which caused the --app-dir option to the litestar CLI to not be propagated during autodiscovery.

Close Issue(s)

Notes

I am unclear what kind of tests should be written for this, since the existing test should capture it, yet I'm not sure why it doesn't.

@sykloid sykloid requested review from a team as code owners September 3, 2023 19:57
Copy link
Member

@abdulhaq-e abdulhaq-e left a comment

Choose a reason for hiding this comment

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

I wrote a test that I will push to your branch. I hope you don't mind.

Also, the env is resolved here:

ctx.obj = lambda: LitestarEnv.from_env(app_path)

I don't know yet the exact conditions on which this line is invoked, but it is important as removing this line causes a lot of test failures. You may want to follow this further.

@abdulhaq-e
Copy link
Member

I've added another test, however that test also passes without having to pass app_dir in

ctx.obj = lambda: LitestarEnv.from_env(app_path)

@cofin

Is this line only used for testing?

if ctx.obj is None: # env has not been loaded yet, so we can lazy load it

Copy link
Member

@abdulhaq-e abdulhaq-e left a comment

Choose a reason for hiding this comment

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

I've added app_dir to the line mentioned in my comments just in case. Other than that, it's all good.

Congratulations on your first contribution to Litestar 👍

@abdulhaq-e abdulhaq-e force-pushed the fix/issue-2266 branch 2 times, most recently from 4fda9b7 to df13c63 Compare September 5, 2023 22:49
@cofin
Copy link
Member

cofin commented Sep 6, 2023

I feel like we need to override the path that's passed into the _autodiscover_app function on this one.

One way to do that would be to override the cwd:

        cwd = Path().cwd() if app_dir is None else Path(app_dir)
        cwd_str_path = str(cwd)
        if cwd_str_path not in sys.path:
            sys.path.append(cwd_str_path)

You would then remove the additional if conditional that was added.

@abdulhaq-e
Copy link
Member

abdulhaq-e commented Sep 6, 2023

I feel like we need to override the path that's passed into the _autodiscover_app function on this one.

One way to do that would be to override the cwd:

        cwd = Path().cwd() if app_dir is None else Path(app_dir)
        cwd_str_path = str(cwd)
        if cwd_str_path not in sys.path:
            sys.path.append(cwd_str_path)

You would then remove the additional if conditional that was added.

I just tried it, lots of failures, all in test_core_commands.py, I think both directories need to be added to the path.

@cofin
Copy link
Member

cofin commented Sep 6, 2023

Nice work!

@cofin
Copy link
Member

cofin commented Sep 6, 2023

@all-contributors add @sykloid for bugs, code, and tests

@allcontributors
Copy link
Contributor

@cofin

I've put up a pull request to add @sykloid! 🎉

@abdulhaq-e abdulhaq-e enabled auto-merge (squash) September 6, 2023 01:08
@abdulhaq-e abdulhaq-e merged commit 6a6fba4 into litestar-org:main Sep 6, 2023
17 checks passed
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2277

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.

Bug: --app-dir option does not work/fails during autodiscovery.
3 participants