-
Notifications
You must be signed in to change notification settings - Fork 43
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
Various changes #96
Various changes #96
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your contribution!
I went through and made some comments.
Regarding the other changes you mentioned in the PR text, I would suggest to open a new issue, so we can discuss how to move forward.
Let me know what you think,
Andreas
Hi @brl0, I opened two new issues for the remaining items you mentioned in the PR text. Cheers, |
Hey @ap--, Thanks so much for your timely and thorough review, I enjoy learning from feedback and really appreciate your time. I made some changes and responded to some of your comments and would love to hear any more of your thoughts. |
Co-authored-by: Andreas Poehlmann <[email protected]>
I thought I'd follow up to see if my changes were acceptable, and to see if you had any more feedback on this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the comments and adding the tests ❤️
There's one small change left, and then we're good to merge this.
Thank you @brl0 ❤️ This is a great improvement. I'll merge this now and will prepare a new release over the weekend. Also thank you so much for your patience 🙇 It's been a few busy weeks over here... |
These changes are broken out from the result of some long running tinkering I started a couple of years ago. To help keep the review process reasonable, I am trying to strike a balance of not creating too many small PRs while also not creating overly large PRs. Along those lines I am willing to break out or combine change sets in whatever manner is most comfortable for review, just let me know.
Changes in this PR:
__new__
, extracting a method for creating paths from other path objects to improve readability and clarity.Changes split out for later:
I'd be eager for any feedback or suggestions. Thanks!