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

166 type hinting #173

Merged
merged 34 commits into from
Jun 4, 2024
Merged

166 type hinting #173

merged 34 commits into from
Jun 4, 2024

Conversation

S-Linde
Copy link
Collaborator

@S-Linde S-Linde commented May 1, 2024

This pull request contains:

  • Type hints for all untyped methods
  • Fixed incorrect type hints
  • Modernized type hints (int | str instead of Union[int, str])

closes #166 and #199

opensquirrel/exporter/writer.py Outdated Show resolved Hide resolved
opensquirrel/exporter/writer.py Outdated Show resolved Hide resolved
@S-Linde S-Linde self-assigned this May 1, 2024
@S-Linde S-Linde requested review from rturrado and juanboschero May 1, 2024 08:58
juanboschero
juanboschero previously approved these changes May 1, 2024
opensquirrel/circuit_builder.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rturrado rturrado left a comment

Choose a reason for hiding this comment

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

Great work, Stan!

Just a comment: maybe better to use List and Tuple from typing? Than list and tuple.

I've got errors with Python 3.8 this afternoon because of using list.

opensquirrel/circuit.py Show resolved Hide resolved
opensquirrel/circuit.py Show resolved Hide resolved
opensquirrel/circuit_builder.py Show resolved Hide resolved
opensquirrel/decomposer/cnot_decomposer.py Show resolved Hide resolved
opensquirrel/decomposer/general_decomposer.py Show resolved Hide resolved
opensquirrel/decomposer/zyz_decomposer.py Show resolved Hide resolved
opensquirrel/exporter/writer.py Outdated Show resolved Hide resolved
opensquirrel/parser/libqasm/libqasm_ir_creator.py Outdated Show resolved Hide resolved
opensquirrel/parser/libqasm/libqasm_ir_creator.py Outdated Show resolved Hide resolved
opensquirrel/utils/identity_filter.py Outdated Show resolved Hide resolved
@S-Linde
Copy link
Collaborator Author

S-Linde commented May 27, 2024

Great work, Stan!

Just a comment: maybe better to use List and Tuple from typing? Than list and tuple.

I've got errors with Python 3.8 this afternoon because of using list.

I specifically used list over List from typing as that version is depricated (see).

This indeed is not immediately compatible with python3.8. However, in 99% of the cases this can be solved by adding from __future__ import annotations at the top of the file. I though I put this on the top of all relevant files, but I might have missed some. I'll take a closer look later this week.

@rturrado
Copy link
Contributor

Great work, Stan!
Just a comment: maybe better to use List and Tuple from typing? Than list and tuple.
I've got errors with Python 3.8 this afternoon because of using list.

I specifically used list over List from typing as that version is depricated (see).

This indeed is not immediately compatible with python3.8. However, in 99% of the cases this can be solved by adding from __future__ import annotations at the top of the file. I though I put this on the top of all relevant files, but I might have missed some. I'll take a closer look later this week.

Oh wow, good catch. I am gonna file the replacement of List to list as a new issue. Thanks!

Copy link
Contributor

@rturrado rturrado left a comment

Choose a reason for hiding this comment

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

Great work, @S-Linde.

I have only seen that merging error with libqasm_ir_creator.py.

However, according to one error I've just seen in the logs, make sure there are no references to squirrel_ir throughout the code as well. squirrel_ir has been renamed to ir now.

opensquirrel/parser/libqasm/libqasm_ir_creator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rturrado rturrado left a comment

Choose a reason for hiding this comment

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

Hi Stan,

Is this ready to go?

I don't know why I still see refrences to squirrel_ir, and a parser/libqasm/libqasm_ir_creator.py file in the Files changed section.

Thanks!

opensquirrel/circuit_builder.py Outdated Show resolved Hide resolved
opensquirrel/instruction_library.py Outdated Show resolved Hide resolved
opensquirrel/parser/libqasm/libqasm_ir_creator.py Outdated Show resolved Hide resolved
rturrado
rturrado previously approved these changes Jun 4, 2024
@S-Linde
Copy link
Collaborator Author

S-Linde commented Jun 4, 2024

Hi Stan,

Is this ready to go?

I don't know why I still see refrences to squirrel_ir, and a parser/libqasm/libqasm_ir_creator.py file in the Files changed section.

Thanks!

It's almost ready. I'm trying to add a github action for mypy as well. If I get that to work, than it's done

@S-Linde
Copy link
Collaborator Author

S-Linde commented Jun 4, 2024

Hi Stan,
Is this ready to go?
I don't know why I still see refrences to squirrel_ir, and a parser/libqasm/libqasm_ir_creator.py file in the Files changed section.
Thanks!

It's almost ready. I'm trying to add a github action for mypy as well. If I get that to work, than it's done

Done

@rturrado rturrado closed this Jun 4, 2024
@rturrado rturrado deleted the 166-type-hinting branch June 4, 2024 12:29
@S-Linde S-Linde restored the 166-type-hinting branch June 4, 2024 12:45
@S-Linde S-Linde reopened this Jun 4, 2024
@rturrado rturrado self-requested a review June 4, 2024 12:49
@S-Linde S-Linde merged commit 08d31cd into develop Jun 4, 2024
34 checks passed
@S-Linde S-Linde deleted the 166-type-hinting branch June 4, 2024 12:50
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.

Type Hinting
3 participants