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 spurious failure of the check/all script #6611

Merged
merged 5 commits into from
May 23, 2024

Conversation

pavoljuhas
Copy link
Collaborator

  • Avoid passing an empty string argument to check/format-incremental
    when invoked from check/all

  • Improve validation of the git-revision argument in check scripts by
    allowing git tags that resolve to a commit

  • Fix invalid use of ? in egrep pattern

  • Make check/mypy pass in the dev.env.txt virtual environment

Problem: check/format-incremental fails when invoked with an
empty string, because it is not a valid git revision.

Solution: Use shell array to store the revision in check/all,
that way it is not passed as a sub-command argument if unset.
Ensure grep matches `?` as a literal character.
Allow git tags which point to a commit.
Problem: check/mypy passes in the mypy.env.txt environment, but fails
on "unnecessary type ignore" in dev.env.txt virtual environment,
because it installs IPython with its type information.

Solution: Ignore IPython in type checks.
@pavoljuhas pavoljuhas requested review from vtomole, cduck and a team as code owners May 21, 2024 23:55
@CirqBot CirqBot added the size: S 10< lines changed <50 label May 21, 2024
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (f246c2b) to head (1fc6c95).
Report is 3 commits behind head on main.

Current head 1fc6c95 differs from pull request most recent head 2063909

Please upload reports for the commit 2063909 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6611   +/-   ##
=======================================
  Coverage   97.81%   97.81%           
=======================================
  Files        1063     1063           
  Lines       91761    91761           
=======================================
+ Hits        89755    89759    +4     
+ Misses       2006     2002    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavoljuhas
Copy link
Collaborator Author

@95-martin-orion - can you PTAL?

Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

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

I haven't looked at this (or Cirq code in general) in a very long time, but nothing about this stands out as obviously wrong to me.

@pavoljuhas pavoljuhas enabled auto-merge (squash) May 23, 2024 22:22
@pavoljuhas pavoljuhas merged commit e4b6ab2 into quantumlib:main May 23, 2024
32 checks passed
@pavoljuhas pavoljuhas deleted the fix-check-all-script branch May 23, 2024 22:49
jselig-rigetti pushed a commit to jselig-rigetti/Cirq that referenced this pull request May 28, 2024
- Avoid passing an empty string argument to `check/format-incremental`
  when invoked from `check/all`

- Improve validation of the git-revision argument in check scripts by
  allowing git tags that resolve to a commit

- Fix invalid use of `?` in egrep pattern

- Make check/mypy pass in the dev.env.txt virtual environment
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
- Avoid passing an empty string argument to `check/format-incremental`
  when invoked from `check/all`

- Improve validation of the git-revision argument in check scripts by
  allowing git tags that resolve to a commit

- Fix invalid use of `?` in egrep pattern

- Make check/mypy pass in the dev.env.txt virtual environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants