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

Enriched scope and path #76

Merged
merged 7 commits into from
Nov 7, 2023
Merged

Conversation

evstratbg
Copy link
Contributor

@evstratbg evstratbg commented Oct 16, 2023

Hi!

I added headers to the scope for get_router_path as I have my custom Router which matches against headers, so-called "header-based versioning", as well as deleted Mount isinstance check, because startlette allows Routers include Routers as paths
Would be cool if you accept this PR and release a patch version of the package

Looking forward to your comments

@evstratbg evstratbg changed the title Update middleware.py Enriched scope and path Oct 16, 2023
@stephenhillier
Copy link
Owner

Hi @evstratbg , I'm curious why the StaticFile test fails when switching the isinstance check from Mount to BasePath. Any idea?

@evstratbg
Copy link
Contributor Author

evstratbg commented Oct 25, 2023

Hi @evstratbg , I'm curious why the StaticFile test fails when switching the isinstance check from Mount to BasePath. Any idea?

Hi! I cant reproduce your issue. Are you sure you meant BasePath and not BaseRoute? with BaseRoute everything works as shown in CI and I dont see any challenges as Mount inherits BaseRoute

Can you share the details of your setup?

@stephenhillier
Copy link
Owner

Yes, I meant BaseRoute. I'm not having an issue, I just noticed that StaticFiles handler setup changed at the same time as moving it to testconf.py. When I change it back, the test fails.

Also, thank you for contributing! Sorry it's taken some time to get to this, things have been extra busy.

@evstratbg
Copy link
Contributor Author

@stephenhillier it hasn't changed, I just moved it to a conftest.py, because it's not a test, but the fixture, that is needed for tests.

What else can I do to get this MR merged?

@stephenhillier
Copy link
Owner

@stephenhillier it hasn't changed, I just moved it to a conftest.py

You made a change to the StaticFile test setup at the same time that you moved the files around. I would like to help support your use case but the tests need to pass without modifications (they currently don't).

Happy to take another look if you can get the tests to pass as-is, or if you can share more about the header based routing I can try to help find a way to support it.

@evstratbg
Copy link
Contributor Author

evstratbg commented Nov 7, 2023

@stephenhillier I don't quite understand what you are talking about, because the latest checks have been passed, all green

I rollbacked my changes to the tests folder to keep it as-is

@stephenhillier
Copy link
Owner

You are correct, they are passing. I will try to do a release as soon as I can this week. Thank you for contributing this.

Just for my own knowledge, do you have any examples of the header based versioning? Is this a library on GitHub or custom middleware?

@stephenhillier stephenhillier merged commit a194f99 into stephenhillier:master Nov 7, 2023
@evstratbg
Copy link
Contributor Author

@stephenhillier thanks!
I am planning to publish it this week. I can ping you here with a link if you dont mind

@evstratbg evstratbg deleted the patch-1 branch November 7, 2023 16:33
@evstratbg
Copy link
Contributor Author

I started preparing the repo, you can find it here
Planning to finish this week

@evstratbg
Copy link
Contributor Author

I started preparing the repo, you can find it here Planning to finish this week

released first version

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.

None yet

2 participants