Skip to content

Conversation

@FloydZ
Copy link
Collaborator

@FloydZ FloydZ commented May 22, 2025

Description

  • this PR adds a .version() function to the BaseEstimator class which should help identifying the exact version a user is using if its unclear.

Review process

from cryptographic_estimators.SDEstimator import SDEstimator

SDE = SDEstimator(n=100, k=50, w=10, include_quantum=True)
SDE.version()

this code should print something like:

Branch:  feat/version_function
Commit:  c82150ef0732d428c599c2ee2d2b9b54880e4ce0
Version: 2.0.0

Please test this function in different scenarios, like if the test file is not in the repo. If the repo is not installed, etc...

Copy link
Collaborator

@Dioprz Dioprz left a comment

Choose a reason for hiding this comment

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

I can't provide quick testing now because I need to set up a Nix dev shell to install the package locally; but for now I would ask you to please change the docstrings to follow the Google style docstrings (or from our docs).

@Dioprz
Copy link
Collaborator

Dioprz commented May 29, 2025

@FloydZ Circling back to this, the code looks good to me; but I have one concern:

Looking at how pip and pypi works during the installation process of a Python package, they don't typically include development files such as the .git/ directory. This means that, while this function could be useful for dev-users, it shouldn't be available for those who just want to consume the library through a pip install cryptographic_estimators command (unfortunately, I can't think of a way to test if that's true because the current package distributed by Pypi, of course, doesn't have this function).

So, if that consequence was by design, I would say: go ahead with the PR. But if that wasn't in the original plans, then I would suggest using another approach: setuptools_scm with its version_file configuration parameter. This tool essentially does the same thing we want: keep track of the library version using Git instead of some manual declaration. However, the version_file parameters also allows a normal but auto-managed python file with the version declared as a variable, so we could print or consume the version in any file of the repo without any low-level trick around .git/.

@FloydZ
Copy link
Collaborator Author

FloydZ commented May 29, 2025

setuptools_scm

that looks much saner. Will have a look tomorrow

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 4, 2025

@FloydZ FloydZ marked this pull request as draft June 25, 2025 11:03
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