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

provide custom DeprecatedOption #3904

Merged
merged 5 commits into from Oct 3, 2022

Conversation

adam-tokarski
Copy link
Contributor

Brief summary of the change made

Fixes #3644

  • introduces custom click's DeprecatedOption
  • This is used for handling --disable_progress_bar
  • It's not covered with any tests yet, because not sure if this approach will defend itself.

somewhat longer background

Changing option to another is easy. But to avoid any issues I wanted to have nicely handled both versions of click option (bonus points for informing that one of them is deprecated). I was not able to find working solution, then found on click's GH issue with linked SO example solution.

Seems to be working, but it's kind of hackish, and I'm not personally very happy of it. But it works now and as long the mentioned issue is opened, there is still a chance that such possibility will be provided. Then we will be able to remove this custom option provided in current PR and use something from package itself.

This is also the reason why I didn't provided any tests for that yet - I want to know if sqlfluff's community (@tunetheweb, @alanmcruickshank?) is happy with proposed solution. If yes, I will provide them.

Are there any other side effects of this change that we should be aware of?

  • nope

Pull Request checklist

  • Please confirm you have completed any of the necessary steps below.

  • Included test cases to demonstrate any code changes, which may be one or more of the following:

    • .yml rule test cases in test/fixtures/rules/std_rule_cases.
    • .sql/.yml parser test cases in test/fixtures/dialects (note YML files can be auto generated with tox -e generate-fixture-yml).
    • Full autofix test cases in test/fixtures/linter/autofix.
    • Other.
  • Added appropriate documentation for the change.

  • Created GitHub issues for any relevant followup/future enhancements if appropriate.

@codecov
Copy link

codecov bot commented Oct 1, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (383cf37) compared to base (2ac8e12).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #3904   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          184       185    +1     
  Lines        14251     14288   +37     
=========================================
+ Hits         14251     14288   +37     
Impacted Files Coverage Δ
src/sqlfluff/cli/click_deprecated_option.py 100.00% <100.00%> (ø)
src/sqlfluff/cli/commands.py 100.00% <100.00%> (ø)

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alanmcruickshank
Copy link
Member

I think this approach is sensible - I agree it's a little hacky, but it seems fairly robust. I agree that setting us up for easier times in the future if a similar feature is merged into click makes sense.

On the testing front: we should either cut out parts of the functions we don't need or put together some basic tests for them. Happy for you to decide which bits to remove, and which bits to test.

Would you be happy to put up a pull request to merge the same code into click itself?

@adam-tokarski
Copy link
Contributor Author

I've already cut some things I don't see very useful and did small cleanup. Will see if anything more can be found. Also I'm going to provide some tests.

Was thinking about putting that into click, but I guess that this would be change which rather modifies the click core than just extends it (in that hacky way). I don't say no, but it may be no so straightforward.

@adam-tokarski adam-tokarski force-pushed the issue_3644 branch 2 times, most recently from 445a423 to fa23b2b Compare October 2, 2022 17:03
This is used for --disable_progress_bar. It's not covered with any tests yet, because not sure if this approach will defend itself.
@adam-tokarski
Copy link
Contributor Author

@alanmcruickshank tests were provided, please take a look on that.

@alanmcruickshank
Copy link
Member

Nice. Looks good 👍

A few tests are failing but I think this PR is otherwise good to go.

@adam-tokarski
Copy link
Contributor Author

@alanmcruickshank
Ah, mypy... I fixed these errors - unfortunately I've needed to set to ignore some of them, but there was also an issue with testing on older Python version, and it's fixed as well.

@alanmcruickshank alanmcruickshank merged commit 56ebcd4 into sqlfluff:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardise --disable_progress_bar naming
2 participants