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

Set minimal click version to 8.0 #30

Closed
wants to merge 1 commit into from
Closed

Set minimal click version to 8.0 #30

wants to merge 1 commit into from

Conversation

vznncv
Copy link

@vznncv vznncv commented Sep 1, 2021

The pull request #27 adds update_min_steps option usage of click._termui_impl.ProgressBar that is available from click 8.0. But existed setup.cfg contains the following restriction: click >= 6.7, !=7.0. It breaks scancode with click<8 version installation. For example: https://github.com/ARMmbed/mbed-os/pull/14981/checks?check_run_id=3470879205

This pull request updates minimal click version in the setup.cfg according changes in the pull request #27.

@pombredanne
Copy link
Member

@vznncv Thank you ++ fort this.
Do you mind to add a DCO signoff (either in your commit or in a message of this PR)?

The pull request #27 adds `update_min_steps` option usage
of click._termui_impl.ProgressBar that is available from click 8.0.
So corresponding requirements should be added to setup.cfg.

Signed-off-by: Konstantin Kochin <vznncv@gmail.com>
@vznncv
Copy link
Author

vznncv commented Sep 1, 2021

Do you mind to add a DCO signoff (either in your commit or in a message of this PR)?

Done

@pombredanne
Copy link
Member

Thanks. Let me think a second to see if we can support both versions as Click is otherwise rather popular and I do not want us to also break scancode-toolkit usage as a library.

@pombredanne
Copy link
Member

I am running a local test first with this:

$ for clk_ver in "8.0.1 8.0.0 7.1.2 7.1.1 7.1 6.7"
> do 
> pip uninstall -y click
> pip install click==$clk_ver
> pytest -vvs -n3
> done

@pombredanne
Copy link
Member

And at the moment Celery which is a dep of ScanCode.io does not (yet) support Click 8+
celery/celery#6861 has it but this is not yet in the current release (https://github.com/celery/celery/blob/v5.1.2/requirements/default.txt#L5 ) .. only in the beta.
So I need in all cases to support both versions. Shikes.

@pombredanne
Copy link
Member

I ran these tests:
/scancode-toolkit$ for clk_ver in 8.0.1 7.1.2 7.1.1 7.1 6.7; do pip uninstall -y click; pip install click==$clk_ver; scancode -i samples/ -n3 --json-pp - ; done
and
commoncode$ for clk_ver in 8.0.1 7.1.2 7.1.1 7.1 6.7; do pip uninstall -y click; pip install click==$clk_ver; pytest -vvs -n3; done

and all pass with this patch:

diff --git a/src/commoncode/cliutils.py b/src/commoncode/cliutils.py
index 4ff8c8e..5a2a3b6 100644
--- a/src/commoncode/cliutils.py
+++ b/src/commoncode/cliutils.py
@@ -267,7 +267,8 @@
             '[%(bar)s]' + ' ' + '%(info)s'
             if bar_template is None else bar_template
         )
-    return progress_class(
+
+    kwargs = dict(
         iterable=iterable,
         length=length,
         fill_char=fill_char,
@@ -281,10 +282,15 @@
         label=label,
         file=file,
         color=color,
-        update_min_steps=update_min_steps,
         width=width,
     )
 
+    pb = progress_class([])
+    if hasattr(pb, 'update_min_steps'):
+        kwargs['update_min_steps'] = update_min_steps
+
+    return progress_class(**kwargs)
+
 
 def fixed_width_file_name(path, max_length=25):
     """

The lines

+    pb = progress_class([])
+    if hasattr(pb, 'update_min_steps'): 

... are duck typing at its best... Not super elegant but effective nonetheless!

So with this patch we can accept all currently supported Click versions and not only > 8

@vznncv
Copy link
Author

vznncv commented Sep 1, 2021

I'm closing this pull requests since @pombredanne has proposed better solution.

@pombredanne Did you consider tox usage instead of configure scripts? Generally it does the same things (virtual environment creation for tests), but has additionally features like generative envlis that allows to test a library with different dependency versions.

@vznncv vznncv closed this Sep 1, 2021
@pombredanne
Copy link
Member

@vznncv let me keep this open for now. Tox in addition would help! +1

Note that ./configure does a setup that's also useful beyond testing

@pombredanne
Copy link
Member

Closing for nexB/skeleton#36

@pombredanne
Copy link
Member

And #31

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.

None yet

2 participants