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

fix: bump click in setup.py and requirements.txt #9299

Merged
merged 2 commits into from
Mar 13, 2020
Merged

fix: bump click in setup.py and requirements.txt #9299

merged 2 commits into from
Mar 13, 2020

Conversation

villebro
Copy link
Member

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

As of #9277 , pip-tools was bumped to 4.5.1, which requires click>=7: link. This is currently at odds with setup.py, which requires click<7.0.0. This PR bumps click to the latest version, which seems to work well with Superset.

TEST PLAN

Tested locally + CI

REVIEWERS

@john-bodley

@mistercrunch
Copy link
Member

The reason it was pinned is because click>=7 forces dashes in subcommands ie superset load-examples instead of superset load_examples.

It's better but disruptive... Will break dockers and helm charts everywhere. I wonder if there's a way to support both with click>=7

@mistercrunch
Copy link
Member

mistercrunch commented Mar 13, 2020

Nice solution to support both fresh as of 19 days ago! pallets/click#1123 (comment)

@villebro
Copy link
Member Author

Thanks for this @mistercrunch , I'll update accordingly!

@codecov-io
Copy link

Codecov Report

Merging #9299 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9299      +/-   ##
==========================================
+ Coverage    58.9%   58.93%   +0.03%     
==========================================
  Files         373      373              
  Lines       12026    12033       +7     
  Branches     2953     2955       +2     
==========================================
+ Hits         7084     7092       +8     
+ Misses       4763     4762       -1     
  Partials      179      179
Impacted Files Coverage Δ
superset-frontend/src/components/Button.jsx 88.88% <0%> (ø) ⬆️
...end/src/SqlLab/components/RunQueryActionButton.jsx
...end/src/SqlLab/components/RunQueryActionButton.tsx 88% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c8c4d6...b4a8f13. Read the comment docs.

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM, not a big fan of keeping the _ but totally understand, for the sake of stability.
I propose to remove this on 1.0.0

@villebro
Copy link
Member Author

Perhaps we should add a TODO to keep track of stuff that will get refactored for 1.0?

@villebro villebro merged commit 91f3cb9 into apache:master Mar 13, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants