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.

Sign up for GitHub

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

Jump to bottom

Removing consider-using-dict-items from lint exclusions and updates #12288

Merged
merged 10 commits into from May 10, 2024
Merged

Removing consider-using-dict-items from lint exclusions and updates #12288

merged 10 commits into from May 10, 2024

Conversation

joesho112358
Copy link
Contributor

Summary

#9614 test for removing consider-using-dict-items

Details and comments

Updates for removing the consider-using-dict-items rule if wanted. Open to suggestions on variable names

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Apr 21, 2024
@coveralls
Copy link

coveralls commented Apr 21, 2024 •
edited

Pull Request Test Coverage Report for Build 9026301042

Details

  • 24 of 30 (80.0%) changed or added relevant lines in 10 files are covered.
  • 24 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 89.622%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/result/models.py 1 4 25.0%
qiskit/result/result.py 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 92.37%
crates/qasm2/src/parse.rs 18 96.69%
Totals Coverage Status
Change from base Build 9023909750: -0.03%
Covered Lines: 62193
Relevant Lines: 69395
💛 - Coveralls

@joesho112358 joesho112358 marked this pull request as ready for review April 22, 2024 02:02
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @enavarro51
  • @Qiskit/terra-core
  • @ajavadia
  • @levbishop
  • @nkanazawa1989
  • @t-imamichi

ihincks
ihincks previously approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

LGTM. The primitive object in question only gained .items() the other day, so thanks for catching!

jakelishman
jakelishman reviewed Apr 25, 2024
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for looking at these.

qiskit/transpiler/target.py Outdated Show resolved Hide resolved
qiskit/visualization/circuit/latex.py Outdated Show resolved Hide resolved
@ihincks
Copy link
Contributor

ihincks commented Apr 25, 2024

I should have been clear in my review description: I was looking mainly at the primitives module and was glad for the change. I did look elsewhere for general correctness, but not with an eye to decide whether the change was a net benefit.

jakelishman
jakelishman reviewed May 9, 2024
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This has gained a couple of minor merge conflicts, but otherwise I think it's good to go.

@joesho112358
Merge branch 'main' into remove-consider-using-dict-items-lint-rule
5fd96f7 
# Conflicts:
#	pyproject.toml
#	test/python/primitives/test_backend_sampler_v2.py
#	test/python/primitives/test_statevector_sampler.py
@joesho112358
quick update for using items after removing lint rule
705ddf0
@joesho112358
Github to GitHub in SECURITY.md
555c86e
@t-imamichi
Copy link
Member

LGTM

jakelishman
jakelishman approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the changes! (Not sure if the typo fixes in SECURITY.md were meant for this PR, but it's fine - they're obviously correct and just minor.)

@jakelishman jakelishman added this pull request to the merge queue May 10, 2024
@jakelishman jakelishman added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels May 10, 2024
Merged via the queue into Qiskit:main with commit 235e581 May 10, 2024
15 checks passed
@joesho112358 joesho112358 deleted the remove-consider-using-dict-items-lint-rule branch May 10, 2024 12:56
raynelfss added a commit to raynelfss/qiskit that referenced this pull request May 12, 2024
@raynelfss
Fix:: Adapt to target changes (Qiskit#12288)
2d46d87 
- Fix lint stray imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@ihincks ihincks ihincks left review comments

@nonhermitian nonhermitian Awaiting requested review from nonhermitian nonhermitian is a code owner

@eggerdj eggerdj Awaiting requested review from eggerdj eggerdj is a code owner

@wshanks wshanks Awaiting requested review from wshanks wshanks is a code owner

@jyu00 jyu00 Awaiting requested review from jyu00 jyu00 is a code owner

Assignees
No one assigned
Labels
Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo type: qa Issues and PRs that relate to testing and code quality
Projects
Contributor Monitoring
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@joesho112358 @coveralls @qiskit-bot @ihincks @t-imamichi @jakelishman