Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Download qhull at build-or-sdist time.#16720

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 4 commits intomatplotlib:masterfromanntzer:dqh
Jan 15, 2021
Merged

Conversation

anntzer
Copy link
Contributor

PR Summary

As suggested in#16571 (comment).

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswelltacaswell changed the titleDownload jquery at build-or-sdist time.Download qhull at build-or-sdist time.Mar 10, 2020
@tacaswell
Copy link
Member

I change the PR title, but the commit message is still miss leading.

tacaswell
tacaswell previously requested changesMar 10, 2020
Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

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

👍 in principle.

Please fix the commit message, anyone can clear this review once that is done.

@anntzer
Copy link
ContributorAuthor

oops, fixed.

@anntzer
Copy link
ContributorAuthor

rebased to use qhull 2020.2.@ianthomas23 feel free to force-push over the last PR to become recorded as author; I copy pasted your qhull_wrap.c from#16571 but couldn't cherry pick the commit as that change was not standalone.

@anntzeranntzerforce-pushed thedqh branch 2 times, most recently from3fc40dc to06c7808CompareJanuary 6, 2021 12:36
@timhoffm
Copy link
Member

@anntzer
Copy link
ContributorAuthor

nice, thanks.

@ianthomas23
Copy link
Member

@ianthomas23 feel free to force-push over the last PR to become recorded as author; I copy pasted your qhull_wrap.c from#16571 but couldn't cherry pick the commit as that change was not standalone.

@anntzer I was thinking that if my name isn't on it, I can't be blamed if anything goes wrong with it in future! But it looks like you have already credited/blamed me 😃

@ianthomas23
Copy link
Member

Do we need to consider what to do with qhull's licence file (COPYING.txt)? We used to keep a copy in theextern directory with the source code, but if we are downloading the code on demand I don't know if a static copy is required or not.

@anntzer
Copy link
ContributorAuthor

I'll let@tacaswell decide on the legal stuff :)

@QuLogic
Copy link
Member

It will be built into the wheels, so we need to keep the license. Clause 2 says you must provide the license for any copies, so we should copy it intoLICENSE/ either permanently or on unpack.

@anntzer
Copy link
ContributorAuthor

anntzer commentedJan 8, 2021
edited
Loading

I now copy the license on unpack, and also just unified the codepaths with the freetype download/cache codepath.

Copy link
Member

@ianthomas23ianthomas23 left a comment
edited
Loading

Choose a reason for hiding this comment

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

I am happy with this from a functional point of view as all triangulation tests perform as expected, and the license file is unpacked into theLICENSE directory. I have just one documentation nitpick and there are some test failures but I think some of these are just timeouts.

(the current version)Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
Copy link
Member

@ianthomas23ianthomas23 left a comment

Choose a reason for hiding this comment

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

Although this wasn't my favoured approach, it does fix the problem and I don't want to hold it up any longer. I am happy for it to be merged.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

Looks good. Slightly better naming/documentation possible.

@jklymak
Copy link
Member

The test failures are spurious I think...

@jklymakjklymak merged commit33ef669 intomatplotlib:masterJan 15, 2021
@anntzeranntzer deleted the dqh branchJanuary 15, 2021 17:43
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ianthomas23ianthomas23ianthomas23 approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

@tacaswelltacaswelltacaswell left review comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

6 participants
@anntzer@tacaswell@timhoffm@ianthomas23@QuLogic@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp