Skip to content

Set minimal click version to 8.0 - #30

Closed
vznncv wants to merge 1 commit into
aboutcode-org:mainfrom
vznncv:fix/min_click_version
Closed

Set minimal click version to 8.0#30
vznncv wants to merge 1 commit into
aboutcode-org:mainfrom
vznncv:fix/min_click_version

Conversation

@vznncv

@vznncv vznncv commented Sep 1, 2021

Copy link
Copy Markdown

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
Copy Markdown
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

vznncv commented Sep 1, 2021

Copy link
Copy Markdown
Author

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

Done

@pombredanne

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

vznncv commented Sep 1, 2021

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
Member

Closing for aboutcode-org/skeleton#36

@pombredanne

Copy link
Copy Markdown
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.

2 participants