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

Validate Python arguments #1940

Merged
merged 4 commits into from
Jul 17, 2024
Merged

Validate Python arguments #1940

merged 4 commits into from
Jul 17, 2024

Conversation

khalatepradnya
Copy link
Collaborator

@khalatepradnya
* Check the expected number of arguments and datatypes in Python AST
692dcf5 
  bridge
* Check that control argument(s) is provided if `.ctrl` attribute is
  used
* Add bugs reported in issues NVIDIA#9, NVIDIA#670, and NVIDIA#1641 as tests
@khalatepradnya khalatepradnya self-assigned this Jul 16, 2024
@khalatepradnya khalatepradnya added bug fix To be listed under Bug Fixes in the release notes python bridge Involves the python bridge to quake labels Jul 16, 2024
@khalatepradnya khalatepradnya added this to the release 0.8.0 milestone Jul 16, 2024
@khalatepradnya khalatepradnya requested a review from bmhowe23 July 16, 2024 20:39
@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jul 16, 2024
@cuda-quantum-bot
Docs preview for PR #1940.
eddcbe2
github-actions bot pushed a commit that referenced this pull request Jul 16, 2024
@cuda-quantum-bot
Docs preview for PR #1940.
2be509c
@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

@sacpis
Copy link
Collaborator

sacpis commented Jul 16, 2024

Just my opinion. Seems like visit_call function is too long (1250 lines). I think it should be refactored.

Reviewing other changes in this PR.

@github-actions GitHub Actions
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jul 16, 2024
@cuda-quantum-bot
Docs preview for PR #1940.
9d76ce5
@khalatepradnya
Copy link
Collaborator Author

Just my opinion. Seems like visit_call function is too long (1250 lines). I think it should be refactored.

Reviewing other changes in this PR.

Agreed. Refactoring opportunity for sure. Do you want to capture this in a separate issue?

@khalatepradnya khalatepradnya enabled auto-merge (squash) July 16, 2024 22:50
@sacpis
Copy link
Collaborator

sacpis commented Jul 16, 2024

Or I can quickly code it up in a PR? Or let me know if you would like to refactor it?

@khalatepradnya
Copy link
Collaborator Author

Or I can quickly code it up in a PR? Or let me know if you would like to refactor it?

Separate PR sounds good. I won't get to it right away, please go ahead.

sacpis
sacpis approved these changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@sacpis sacpis left a comment

Choose a reason for hiding this comment

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

Thanks @khalatepradnya. LGTM!

@sacpis
Copy link
Collaborator

sacpis commented Jul 16, 2024

Once this PR is merged in, mind closing the associated issues?

@khalatepradnya
Copy link
Collaborator Author

Once this PR is merged in, mind closing the associated issues?

Will do.

@khalatepradnya khalatepradnya merged commit a674f66 into NVIDIA:main Jul 17, 2024
125 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2024
@khalatepradnya khalatepradnya deleted the python-datatypes branch July 17, 2024 00:22
@bettinaheim bettinaheim changed the title [Bug-fix] [Python] Check arguments Validate Python arguments Jul 29, 2024
@bettinaheim bettinaheim added release notes Changes need to be captured in the release notes user experience Issue or PR that significantly impacts user experience and removed bug fix To be listed under Bug Fixes in the release notes labels Jul 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sacpis sacpis sacpis approved these changes

@anthony-santana anthony-santana Awaiting requested review from anthony-santana

@bmhowe23 bmhowe23 Awaiting requested review from bmhowe23

Assignees

@khalatepradnya khalatepradnya

Labels
python bridge Involves the python bridge to quake release notes Changes need to be captured in the release notes user experience Issue or PR that significantly impacts user experience
Projects
None yet
Milestone
release 0.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@khalatepradnya @sacpis @bettinaheim