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

Rename Builder to BaseBuilder #680

Closed
wants to merge 2 commits into from
Closed

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Jan 4, 2024

A small change of class name. While working on #679 It was confusing to have poetry.core.masonry.builders.builder and poetry.core.masonry.builder have the same name for 2 different classes.

@bswck
Copy link
Member

bswck commented Jan 4, 2024

Thumbs up on that one, I got confused too.

@Secrus
Copy link
Member Author

Secrus commented Jan 5, 2024

Damn, the downstream...

@radoering
Copy link
Member

Copy link

sonarcloud bot commented Jan 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dimbleby
Copy link
Contributor

dimbleby commented Jan 5, 2024

another option - because BaseBuilder is such an ugly name! - would be simply to deprecate and remove the poetry.core.masonry.builder altogether

if I am right it is used only by the poetry build command. and it is so trivial that the code could reasonably just live in poetry proper all along: there's little value in poetry-core exposing a class for this.

@radoering
Copy link
Member

BaseBuilder is such an ugly name

full ack

would be simply to deprecate and remove the poetry.core.masonry.builder altogether

I like that idea because even if we rename the class there are still the modules poetry.core.masonry.builders.builder and poetry.core.masonry.builder, which is confusing enough.

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.

4 participants