Skip to content

Fix #88: add boundbox operation #90

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

Closed
wants to merge 3 commits into from
Closed

Fix #88: add boundbox operation #90

wants to merge 3 commits into from

Conversation

juliohm
Copy link
Member

@juliohm juliohm commented Sep 24, 2020

This PR refactors the bounding box operation as discussed in #88. After it is reviewed and merged, I can start working on other improvements to the codebase.

@juliohm
Copy link
Member Author

juliohm commented Sep 24, 2020

Hi @SimonDanisch is this PR ok? I am waiting on your approval to continue with more PRs.

@SimonDanisch
Copy link
Member

Iooks ok, although will be a relatively big breaking change ;)
Why the rename to boundbox.jl, though?

@juliohm
Copy link
Member Author

juliohm commented Sep 25, 2020

Iooks ok, although will be a relatively big breaking change ;)

I think we will need some more like these before a GeometryBasics.jl v1.0.0 :) I am doing my best to break things as early as possible in favor of a cleaner, more maintainable codebase.

Why the rename to boundbox.jl, though?

I thought that boundingboxes.jl was too long, but I can undo the renaming, would you prefer it? Meanwhile I will change it to boundboxes.jl (plural) to follow the convention of other files.

@juliohm
Copy link
Member Author

juliohm commented Sep 25, 2020

Done :) Please let me know if you prefer the longer boundingboxes.jl and I can push the change as well before merging. Otherwise, feel free to merge and I will start working on a new PR for #91.

@SimonDanisch
Copy link
Member

Please let me know if you prefer the longer boundingboxes.jl

Yeah, definitely prefer the longer one :) No need to be greedy here ;)

@SimonDanisch
Copy link
Member

Master needs to be always taggable - since I don't immediately want to tag a breaking 1.0, I'd prefer not merging any of these.
If you want to combine all your breaking changes into one branch, I propose creating a v1_release branch, into which you can combine all your changes ;)

@juliohm
Copy link
Member Author

juliohm commented Sep 25, 2020

So it is better to tag breaking changes in a separate branch? Could you please advise on how I should proceed?

I understood I should close this PR, and instead work on a separate branch of the repo until things settle down. Isn't it more painful to break everything at once when this big merge occurs as opposed to little breaking changes every now and then? The plan is to tag v0.4, v0.5, v0.6, ... until we get stable enough for a 1.0. Would that be ok?

I will rename back to boundingboxes.jl as suggested.

@SimonDanisch
Copy link
Member

tagging a breaking releases has lots of organisational overhead for me, due to all the dependencies of GeometryBasics, which I mostly maintain alone. Since the changes are usually relatively small per package, I prefer to do multiple ones in one go.
If it's foreseeable, that we do ~3 breaking changes, I'd rather put them into one tag ;) If we don't really know what's coming over a longer time range, we could apply your tactic.

I'd just create a new branch, and change the base of this branch against that, which should be pretty easy with github ;)

@juliohm
Copy link
Member Author

juliohm commented Sep 25, 2020

Makes sense. I will try to follow the rebase instructions to move the changes over a new branch.

@juliohm juliohm closed this Sep 25, 2020
@juliohm juliohm deleted the bbox branch September 25, 2020 11:28
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.

2 participants