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

🔧🚇 Only run check-maifest on the CI #967

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

s-weigand
Copy link
Member

@s-weigand s-weigand commented Jan 24, 2022

While it is important that we check if all files we want to bundle are actually bundled in the distribution, checking this in pre-commit can be cumbersome.
E.g. you were working on a PR and when you got it to work the way you want you to commit different small changes in contextual closed commits. If you added new files which you don't want to add in this commit check-manifest won't allow you to make this commit since it looks at all files and not only the tracked ones.
This in turn means that you need to make big commits or use the --no-verify flag which both can lead to bad commits.

Change summary

  • 🔧🚇 Only run check-manifest on the CI

Checklist

  • ✔️ Passing the tests (mandatory for all PR's)
  • 🚧 Added changes to changelog (mandatory for all PR's)

@s-weigand s-weigand requested a review from a team as a code owner January 24, 2022 02:57
@github-actions
Copy link
Contributor

Binder 👈 Launch a binder notebook on branch s-weigand/pyglotaran/manifest_check_ci_only

@sonarqubecloud
Copy link

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
1.0% 1.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2022

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

Benchmark diff v0.5.1 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)

All benchmarks:

       before           after         ratio
     [96b42630]       [20dd42df]
     <v0.5.1>                   
         62.7±2ms         61.5±1ms     0.98  BenchmarkOptimize.time_optimize(False, False, False)
        92.9±30ms         114±40ms    ~1.22  BenchmarkOptimize.time_optimize(False, False, True)
       62.7±0.8ms       61.5±0.6ms     0.98  BenchmarkOptimize.time_optimize(False, True, False)
        71.5±20ms         65.8±2ms     0.92  BenchmarkOptimize.time_optimize(False, True, True)
         78.9±2ms       73.3±0.2ms     0.93  BenchmarkOptimize.time_optimize(True, False, False)
        82.5±30ms         110±40ms    ~1.34  BenchmarkOptimize.time_optimize(True, False, True)
       74.8±0.6ms       73.8±0.6ms     0.99  BenchmarkOptimize.time_optimize(True, True, False)
        83.1±30ms        88.1±20ms     1.06  BenchmarkOptimize.time_optimize(True, True, True)
             208M             203M     0.98  IntegrationTwoDatasets.peakmem_optimize
       2.12±0.04s       2.02±0.04s     0.95  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)

All benchmarks:

       before           after         ratio
     [1667b7d8]       [20dd42df]
         62.6±1ms         61.5±1ms     0.98  BenchmarkOptimize.time_optimize(False, False, False)
        99.1±40ms         114±40ms    ~1.15  BenchmarkOptimize.time_optimize(False, False, True)
       61.0±0.3ms       61.5±0.6ms     1.01  BenchmarkOptimize.time_optimize(False, True, False)
        68.6±30ms         65.8±2ms     0.96  BenchmarkOptimize.time_optimize(False, True, True)
       73.8±0.6ms       73.3±0.2ms     0.99  BenchmarkOptimize.time_optimize(True, False, False)
         81.5±3ms         110±40ms    ~1.35  BenchmarkOptimize.time_optimize(True, False, True)
       73.8±0.1ms       73.8±0.6ms     1.00  BenchmarkOptimize.time_optimize(True, True, False)
        85.0±40ms        88.1±20ms     1.04  BenchmarkOptimize.time_optimize(True, True, True)
             206M             203M     0.99  IntegrationTwoDatasets.peakmem_optimize
       2.10±0.07s       2.02±0.04s     0.96  IntegrationTwoDatasets.time_optimize

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #967 (20dd42d) into main (1667b7d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #967   +/-   ##
=====================================
  Coverage   85.3%   85.3%           
=====================================
  Files         90      90           
  Lines       4859    4859           
  Branches     913     913           
=====================================
  Hits        4149    4149           
  Misses       561     561           
  Partials     149     149           

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 1667b7d...20dd42d. Read the comment docs.

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.

Good idea. 👍

@jsnel jsnel merged commit 707c227 into glotaran:main Jan 24, 2022
@s-weigand s-weigand deleted the manifest_check_ci_only branch January 24, 2022 08:23
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