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

Refactor add_path even more #238

Merged
merged 5 commits into from Jul 14, 2018

Conversation

@lafrech
Copy link
Member

commented Jul 6, 2018

This is based on #237 and goes further. Path is removed.

Clearer API (IMHO), and almost no test broken.

Probably not perfect, but I think it is a nice improvement.

@sloria, I'd understand your reluctance to introduce breaking changes. Would it make you feel more comfortable to publish a 1.0 version then break everything in 2.x? I don't mind either way. The breaking changes introduced here are not much worse than those introduced with the class interface. They will break every custom path helper, and possibly some operation helpers, marginally, but the fix is easy. It is not as if it made some feature impossible.

@lafrech lafrech changed the title WIP - Refactor add_path even more Refactor add_path even more Jul 13, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2018

I shouldn't have labelled this WIP.

@sloria, I don't mean to rush you if you saw that but didn't have the time to look at it. If it was non-breaking I would merge it myself and wait for you to release, but since it is a bit more than that, I'm requesting your review.

@lafrech lafrech requested a review from sloria Jul 13, 2018

@sloria

sloria approved these changes Jul 14, 2018

Copy link
Member

left a comment

Yeah, I saw this and just hadn't had the time to review it in earnest.

I'm good with this change. It was admittedly pretty ugly to allow operations to be passed both in Path and as a kwarg to add_path, so I'm all for cleaning that up. And +1 for reducing the API surface.

How about we release this in 0.40.0 then remove all deprecated API in 1.0.0?

@sloria sloria merged commit 1d68247 into dev Jul 14, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details

@sloria sloria deleted the dev_refactor_add_path_even_more branch Jul 14, 2018

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 15, 2018

Actually, let's make this 1.0.0b1 then remove all deprecated API in 1.0.0 final.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

Yeah, I saw this and just hadn't had the time to review it in earnest.

Sorry for rushing you on this. I wasn't sure how much you rely on @ ping and/or review requests to reach your attention. (I don't.)

Actually, let's make this 1.0.0b1 then remove all deprecated API in 1.0.0 final.

As you wish. We can remove all deprecated stuff right now since we're now in beta: it won't be pulled unless asked explicitly with --pre.

I had a few other things in mind. I'm not sure they'll make it to v1.0 but if these are breaking changes, that'll be a 2.x.

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

I want to give users an easy transition to 1.0, hence allowing for one beta release with the deprecated API. We can remove it in 1.0.0b2 if we end up needing another beta.

Feel free to tag any issues with the 1.0 milestone. Otherwise, I'll just plan on releasing 1.0.0 as is, sans deprecated API, in a week or two.

@alysivji alysivji referenced this pull request Jul 26, 2018
@alysivji

This comment has been minimized.

Copy link

commented Jul 29, 2018

@sloria I'm writing a plugin for falcon which is using the new add_path. When will the 1.0.0bx release be up on PyPI?

Thanks for this great library!

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

@alysivji I've gone ahead and cut the 1.0.0b1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.