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

Beautify code #97

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

andrew000
Copy link

  • Move project to UV; https://docs.astral.sh/uv
  • Add static type checker MyPy
  • Add linter and code formatter ruff
  • Add pre-commit
  • Reformat code using ruff
  • Make code more secure (stop using os.path, os.system)
  • Update workflow python_simplified.yml
  • Update workflow python_detailed.yml
  • Drop py3.8, because of EOL.

Drop workflow python version from 3.10 to 3.9 to be sure that the library will work on py3.9+. Also, I want to add matrix to test the library on 3.9, 3.10, 3.11, 3.12, 3.13, but I don't see anything deprecated in 3.9 that can't be in 3.13 in this code.

Add typing
Add pre-commit
Reformat code
Make code more secure
Update workflow python_simplified.yml
Update workflow python_detailed.yml

Signed-off-by: andrew000 <[email protected]>
oqs/oqs.py Outdated Show resolved Hide resolved
oqs/oqs.py Outdated Show resolved Hide resolved
@vsoftco
Copy link
Member

vsoftco commented Jan 14, 2025

I will merge this after 0.12 release (later this week)

@andrew000
Copy link
Author

I will merge this after 0.12 release (later this week)

After accepting #100 I will refactor this PR to apply changes from #100 .

@vsoftco vsoftco force-pushed the main branch 5 times, most recently from 981bf5c to 7906e78 Compare January 16, 2025 04:02
@vsoftco
Copy link
Member

vsoftco commented Jan 21, 2025

@andrew000 We've released 0.12. Updated the default examples to use ML-KEM/DSA. Please feel free to resolve the other conflicts then we can merge the PR.

@andrew000
Copy link
Author

I will resolve conflicts later this week

# Conflicts:
#	examples/kem.py
#	examples/sig.py
#	oqs/oqs.py
#	oqs/rand.py
#	pyproject.toml
#	tests/test_kem.py
#	tests/test_sig.py
Bump deps

Signed-off-by: andrew000 <[email protected]>
Add stream handler to logger

Signed-off-by: andrew000 <[email protected]>
@andrew000 andrew000 requested a review from vsoftco January 26, 2025 18:57
@vsoftco
Copy link
Member

vsoftco commented Jan 27, 2025

@andrew000 Thanks, I will review shortly and merge

@vsoftco
Copy link
Member

vsoftco commented Jan 27, 2025

@andrew000 I think we need something like logger.addHandler(logging.StreamHandler(sys.stdout)) in the examples, so we can filter out warnings from oqs (like the mismatch between oqs and oqs-python) with python examples/kem.py 2>/dev/null. Otherwise, everything goes to stderr, which should not be the intended behaviour in the examples.

Alternatively, keep the good ol' print.

@vsoftco
Copy link
Member

vsoftco commented Jan 29, 2025

@andrew000 I've modified the examples (see the last commit). Let me know if this looks OK for you, and I'll merge the PR.

@andrew000
Copy link
Author

@andrew000 I've modified the examples (see the last commit). Let me know if this looks OK for you, and I'll merge the PR.

Tonight I will review the changes and fix some things if I find anything.

@vsoftco
Copy link
Member

vsoftco commented Jan 31, 2025

@andrew000 I'm getting some Pyright type errors for _fields_, L270 and L467 in oqs.py:

"_fields_" overrides symbol of same name in class "Structure" │   Variable is mutable so its type is invariant │     Override type "list[tuple[str, Any]]" is not the same as base type "Sequence[tuple[str, type[_SimpleCData[Any]] | type[_Pointer[Any]] | type[CFuncPtr] | type[Union] | type[Structure] | type[Array[Any]]] | tuple[str, type[_SimpleCData[Any]] | type[_Pointer[Any]] | type[CFuncPtr] | type[Union] | type[Structure] | type[Array[Any]], int]]" Pyright (reportIncompatibleVariableOverride) [270, 5]

I guess it's the static type checker being unhappy, but I'm not too familiar with types in Python. Any clues?

@andrew000
Copy link
Author

@andrew000 I'm getting some Pyright type errors for _fields_, L270 and L467 in oqs.py:

"_fields_" overrides symbol of same name in class "Structure" │   Variable is mutable so its type is invariant │     Override type "list[tuple[str, Any]]" is not the same as base type "Sequence[tuple[str, type[_SimpleCData[Any]] | type[_Pointer[Any]] | type[CFuncPtr] | type[Union] | type[Structure] | type[Array[Any]]] | tuple[str, type[_SimpleCData[Any]] | type[_Pointer[Any]] | type[CFuncPtr] | type[Union] | type[Structure] | type[Array[Any]], int]]" Pyright (reportIncompatibleVariableOverride) [270, 5]

I guess it's the static type checker being unhappy, but I'm not too familiar with types in Python. Any clues?

https://docs.python.org/3/library/ctypes.html#ctypes.Structure

Concrete structure and union types must be created by subclassing one of these types, and at least define a _fields_ class variable.

Signed-off-by: andrew000 <[email protected]>
@andrew000
Copy link
Author

@andrew000 I've modified the examples (see the last commit). Let me know if this looks OK for you, and I'll merge the PR.

LGTM.

I have updated ruff in last commit.

So I think PR is ready to be merged 👍

@andrew000
Copy link
Author

Documentation does not provide any detailed type hints.

typeshed declaring a type hint:

class _StructUnionMeta(_CDataMeta):
    _fields_: Sequence[tuple[str, type[_CData]] | tuple[str, type[_CData], int]]

But _CData is not declared in python sources.

Finally we can try change list to Sequence to try to calm down static type checker

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.

3 participants