Skip to content

build: Update lower bound of click to v8.0.0#1958

Merged
kratsg merged 3 commits intomasterfrom
build/update-click-lower-bound
Aug 26, 2022
Merged

build: Update lower bound of click to v8.0.0#1958
kratsg merged 3 commits intomasterfrom
build/update-click-lower-bound

Conversation

@matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Aug 26, 2022 •
edited
Loading

Description

Resolves #1957

edit: @kratsg has found in #1957 (comment) that Click v7 has a bug and didn't set the path type correctly.

  • Update

pyhf/tests/constraints.txt

Line 3 in f01dcf5

click==7.0

to use click==8.0.0.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Update lower bound of the supported click versions to v8.0.0 as it
is required to support behavior in PR #1909 due to click v7.x not
setting the type correctly for path type arguments.
   - c.f. https://github.com/scikit-hep/pyhf/issues/1957#issuecomment-1228782947
* Update tests/constraints.txt to use click==8.0.0.

@matthewfeickert matthewfeickert added fix A bug fix build Changes that affect the build system or external dependencies dependencies Pull requests that update a dependency file labels Aug 26, 2022
@matthewfeickert matthewfeickert self-assigned this Aug 26, 2022
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 08:27 Inactive
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 08:37 Inactive
@codecov
Copy link

codecov bot commented Aug 26, 2022 •
edited
Loading

Codecov Report

Merging #1958 (096d967) into master (3ac885e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1958   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files          68       68           
  Lines        4380     4380           
  Branches      728      728           
=======================================
  Hits         4303     4303           
  Misses         45       45           
  Partials       32       32
Flag Coverage Δ
contrib 26.55% <ø> (ø)
doctest 60.54% <ø> (ø)
unittests-3.10 96.14% <ø> (ø)
unittests-3.7 96.12% <ø> (ø)
unittests-3.8 96.16% <ø> (ø)
unittests-3.9 96.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Aug 26, 2022

These failing 3 tests are for click v7.1, v7.1.1, and v7.1.2, and the passing test is for v8.0.0

failing-passing

@kratsg kratsg mentioned this pull request Aug 26, 2022
Closed
@matthewfeickert matthewfeickert marked this pull request as draft August 26, 2022 16:05
@matthewfeickert matthewfeickert marked this pull request as ready for review August 26, 2022 16:13
@matthewfeickert
build: Update lower bound of click to v8.0.0
bb8665a 
* Update lower bound of the supported click versions to v8.0.0
as it is required to support behavior in PR #1909.
* Update tests/constraints.txt to use click==8.0.0.
@matthewfeickert
Add PR note
347cb79
@matthewfeickert
fix number
096d967
@matthewfeickert matthewfeickert force-pushed the build/update-click-lower-bound branch from 7c00d46 to 096d967 Compare August 26, 2022 16:13
@matthewfeickert matthewfeickert temporarily deployed to github-pages August 26, 2022 16:13 Inactive
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Aug 26, 2022

We now know from #1957 (comment) that click as a project only supports the latest release (currently 8.1.x) and so trying to diagnose a v7 issue won't be something that we should expect any engagement on with the click team.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Aug 26, 2022 •
edited
Loading

Note: @kratsg has found in #1957 (comment) that between v7 and v8 the path type changed in click.

@kratsg
Copy link
Contributor

kratsg commented Aug 26, 2022

Note: @kratsg has found in #1957 (comment) that between v7 and v8 the path type changed in click.

No, this is wrong. Click has a bug and didn't set the path type correctly in v7.0.

@kratsg kratsg merged commit fc9ffd1 into master Aug 26, 2022
@kratsg kratsg deleted the build/update-click-lower-bound branch August 26, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kratsg kratsg kratsg approved these changes

@lukasheinrich lukasheinrich Awaiting requested review from lukasheinrich

Assignees

@matthewfeickert matthewfeickert

Labels

build Changes that affect the build system or external dependencies dependencies Pull requests that update a dependency file fix A bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Minimum supported version tests failing

2 participants

@matthewfeickert @kratsg