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/cli0.5 #765

Merged
merged 9 commits into from
Aug 13, 2021
Merged

Fix/cli0.5 #765

merged 9 commits into from
Aug 13, 2021

Conversation

joernweissenborn
Copy link
Member

@joernweissenborn joernweissenborn commented Aug 8, 2021

This PR fixes up the CLI.

Change summary

  • Adapt CLI to API changes
  • Add CLI tests

Checklist

  • ✔️ Passing the tests (mandatory for all PR's)
  • 👌 Closes issue (mandatory for ✨ feature and 🩹 bug fix PR's)
  • 🧪 Adds new tests for the feature (mandatory for ✨ feature and 🩹 bug fix PR's)
  • 📚 Adds documentation of the feature

Closes issues

closes #719

@joernweissenborn joernweissenborn requested review from jsnel and a team as code owners August 8, 2021 16:27
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2021

Binder 👈 Launch a binder notebook on branch joernweissenborn/pyglotaran/fix/cli0.5

@codecov
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #765 (f4e607c) into staging (cee1834) will increase coverage by 2.3%.
The diff coverage is 77.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           staging    #765     +/-   ##
=========================================
+ Coverage     80.8%   83.2%   +2.3%     
=========================================
  Files           71      72      +1     
  Lines         4061    4081     +20     
  Branches       730     736      +6     
=========================================
+ Hits          3285    3397    +112     
+ Misses         645     550     -95     
- Partials       131     134      +3     
Impacted Files Coverage Δ
glotaran/cli/commands/optimize.py 25.3% <50.0%> (+25.3%) ⬆️
glotaran/cli/commands/util.py 35.6% <71.4%> (+35.6%) ⬆️
glotaran/cli/__init__.py 100.0% <100.0%> (ø)
glotaran/cli/commands/pluginlist.py 100.0% <100.0%> (+100.0%) ⬆️
glotaran/cli/main.py 100.0% <100.0%> (+100.0%) ⬆️
glotaran/project/scheme.py 90.0% <100.0%> (ø)
glotaran/cli/commands/print.py 21.0% <0.0%> (+21.0%) ⬆️
... and 3 more

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 cee1834...f4e607c. Read the comment docs.

@s-weigand
Copy link
Member

The CLI also should have some tests, so if it breaks again we actually see it.
click already has a framework for that, dummy example below.

from glotaran import cli
from click.testing import CliRunner

def test_command_line_interface():
    """Test the CLI."""
    runner = CliRunner()
    result = runner.invoke(cli.main)
    assert result.exit_code == 0
    assert "glotaran.cli.main" in result.output
    help_result = runner.invoke(cli.main, ["--help"])
    assert help_result.exit_code == 0
    assert "--help  Show this message and exit." in help_result.output

Copy link
Member

@s-weigand s-weigand left a comment

Choose a reason for hiding this comment

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

There is still room for more fixes 😛

glotaran/cli/commands/optimize.py Outdated Show resolved Hide resolved
glotaran/cli/commands/optimize.py Outdated Show resolved Hide resolved
glotaran/cli/commands/optimize.py Outdated Show resolved Hide resolved
glotaran/cli/commands/pluginlist.py Outdated Show resolved Hide resolved
glotaran/cli/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@jsnel jsnel left a comment

Choose a reason for hiding this comment

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

Left some comments suggestion to avoid importing glotaran as gta and to call the entrypoint of the CLI main.

Avoid using import glotaran as gta
Replace calls to deprecated function
@s-weigand s-weigand requested a review from jsnel August 13, 2021 12:16
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Aug 13, 2021

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 0.09%.

Quality metrics Before After Change
Complexity 8.96 🙂 8.79 🙂 -0.17 👍
Method Length 45.15 ⭐ 46.06 ⭐ 0.91 👎
Working memory 10.99 😞 11.67 😞 0.68 👎
Quality 65.03% 🙂 64.94% 🙂 -0.09% 👎
Other metrics Before After Change
Lines 419 461 42
Changed files Quality Before Quality After Quality Change
glotaran/cli/init.py % % %
glotaran/cli/main.py 89.95% ⭐ 89.63% ⭐ -0.32% 👎
glotaran/cli/commands/optimize.py 12.59% ⛔ 11.73% ⛔ -0.86% 👎
glotaran/cli/commands/pluginlist.py 87.13% ⭐ 86.81% ⭐ -0.32% 👎
glotaran/cli/commands/util.py 73.09% 🙂 73.47% 🙂 0.38% 👍
glotaran/project/scheme.py 90.75% ⭐ 90.75% ⭐ 0.00%

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
glotaran/cli/commands/optimize.py optimize_cmd 29 😞 457 ⛔ 29 ⛔ 11.73% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
glotaran/cli/commands/util.py select_data 9 🙂 131 😞 9 🙂 58.32% 🙂 Try splitting into smaller methods
glotaran/cli/commands/util.py ValOrRangeOrList.convert 6 ⭐ 88 🙂 11 😞 62.86% 🙂 Extract out complex expressions
glotaran/cli/commands/util.py write_data 1 ⭐ 42 ⭐ 10 😞 76.78% ⭐ Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@sonarcloud
Copy link

sonarcloud bot commented Aug 13, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@s-weigand s-weigand self-requested a review August 13, 2021 12:17
Copy link
Member

@s-weigand s-weigand left a comment

Choose a reason for hiding this comment

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

I resolved the issues I found, so @joernweissenborn and @jsnel release review my changes.

@github-actions
Copy link
Contributor

Benchmark is done. Checkout the benchmark result page.
Benchmark differences below 5% might be due to CI noise.

Benchmark diff

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)

All benchmarks:

       before           after         ratio
     [dc00e6da]       [f4e607c3]
     <v0.4.0>                   
-        61.5±1ms         42.5±2ms     0.69  BenchmarkOptimize.time_optimize(False, False, False)
-         341±3ms        68.5±20ms     0.20  BenchmarkOptimize.time_optimize(False, False, True)
-      88.5±0.9ms         73.8±2ms     0.83  BenchmarkOptimize.time_optimize(False, True, False)
       89.1±0.8ms         84.9±1ms     0.95  BenchmarkOptimize.time_optimize(False, True, True)
         61.3±1ms       56.9±0.6ms     0.93  BenchmarkOptimize.time_optimize(True, False, False)
-         337±4ms         68.9±1ms     0.20  BenchmarkOptimize.time_optimize(True, False, True)
         87.5±1ms         92.1±2ms     1.05  BenchmarkOptimize.time_optimize(True, True, False)
         91.6±1ms         137±10ms    ~1.49  BenchmarkOptimize.time_optimize(True, True, True)
             181M             180M     1.00  IntegrationTwoDatasets.peakmem_create_result
             198M             198M     1.00  IntegrationTwoDatasets.peakmem_optimize
-         287±3ms          223±4ms     0.78  IntegrationTwoDatasets.time_create_result
-      5.78±0.01s       1.70±0.05s     0.29  IntegrationTwoDatasets.time_optimize

Copy link
Member

@jsnel jsnel left a comment

Choose a reason for hiding this comment

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

Still needs a bit better test coverage but otherwise ok for merge now.

@jsnel jsnel merged commit bef7262 into glotaran:staging Aug 13, 2021
@joernweissenborn joernweissenborn deleted the fix/cli0.5 branch August 13, 2021 15:30
jsnel added a commit that referenced this pull request Aug 14, 2021
Various fixes and improvements to the glotaran command line interface.

* Changed CLI save plugin to folder
* Added outputformat option to CLI
* Added basic test for CLI
* Rename CLI entrypoint to main and add more CLI tests
* 👌 CLI use same default for non_negative_least_squares as scheme
* 👌 CLI dedent pluginlist output
* 🩹 CLI fixed result outformat accepting none supported formats

Co-authored-by: Joris Snellenburg <[email protected]>
Co-authored-by: s-weigand <[email protected]>
jsnel added a commit to jsnel/pyglotaran that referenced this pull request Aug 14, 2021
Various fixes and improvements to the glotaran command line interface.

* Changed CLI save plugin to folder
* Added outputformat option to CLI
* Added basic test for CLI
* Rename CLI entrypoint to main and add more CLI tests
* 👌 CLI use same default for non_negative_least_squares as scheme
* 👌 CLI dedent pluginlist output
* 🩹 CLI fixed result outformat accepting none supported formats

Co-authored-by: Joris Snellenburg <[email protected]>
Co-authored-by: s-weigand <[email protected]>
jsnel added a commit that referenced this pull request Sep 16, 2021
Various fixes and improvements to the glotaran command line interface.

* Changed CLI save plugin to folder
* Added outputformat option to CLI
* Added basic test for CLI
* Rename CLI entrypoint to main and add more CLI tests
* 👌 CLI use same default for non_negative_least_squares as scheme
* 👌 CLI dedent pluginlist output
* 🩹 CLI fixed result outformat accepting none supported formats

Co-authored-by: Joris Snellenburg <[email protected]>
Co-authored-by: s-weigand <[email protected]>
s-weigand added a commit to regro-cf-autotick-bot/pyglotaran-feedstock that referenced this pull request Dec 2, 2021
…ests

The problem is that it seams to pick up the command script of an older version leading to the error:
ImportError: cannot import name 'glotaran' from 'glotaran.cli.main'

'glotaran.cli.main.glotaran' was renamed to 'glotaran.cli.main.main' in this PR
glotaran/pyglotaran#765
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.

3 participants