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

feat(LineSegment2D): Add is_equivalent method to line2d and tests. #142

Merged
merged 1 commit into from
Jul 4, 2020

Conversation

saeranv
Copy link
Member

@saeranv saeranv commented Jul 2, 2020

@chriswmackey

I'd like to add this is_equivalent method to the LineSegment2D object. Note that order of the points doesn't matter for equivalence to be True.

@chriswmackey chriswmackey self-requested a review July 3, 2020 20:35
Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

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

This looks good @saeranv but do you mind if I merge this in with a different semantic release tag? If I merge this as is, it's going to trigger a re-release of everything that depends on ladybug-geometry, which is over 10 repos. Unless you have something in one of these other repos that depends on this new method, I would rather not do a whole new release.

@saeranv
Copy link
Member Author

saeranv commented Jul 3, 2020

@chriswmackey

I need this for updates to some revised equivalence checks in the ladybug-geometry-polyskel repo, but I admit I don't understand if or how the semantic release tag effects that.

As long as I do another pip install ladybug-geometry -U once this commit is pushed. Shouldn't these changes therefore be correctly propagated to all my other dependent repos (in this case the ladybug-geometry-polyskel one), regardless of the tag?

@chriswmackey
Copy link
Member

Here's the core library dependency tree:
Ladybug Tools Repo Structures (4)

Making a release at the roots without updating the requirements.txt of the higher branches will cause everything above it to be out of sync. So you will get compatibility warnings when running pip commands like the one you cite there. To avoid this case, our CI makes a new release of everything above a given library whenever something is merged to in order to ensure there's always a compatible set of all the core libraries on PyPI.

Long story short: do you mind getting together your PR for the polyskel library first before we merge this? I just want to avoid the case that you find that other changes need to be made to ladybug-geometry while you're editing ladybug-geomtery-polyskel and I really don't want to do an extra release of everything if I can avoid it.

@saeranv
Copy link
Member Author

saeranv commented Jul 4, 2020

@chriswmackey

Ah I understand, I already have the code done for the polyskel so I can PR it. I don't think I'll need any more changes to this repo.

@chriswmackey chriswmackey merged commit a3b42df into ladybug-tools:master Jul 4, 2020
@ladybugbot
Copy link

🎉 This PR is included in version 1.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants