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

finish six removal #544

Merged
merged 1 commit into from
Nov 14, 2024
Merged

finish six removal #544

merged 1 commit into from
Nov 14, 2024

Conversation

a-detiste
Copy link
Contributor

Hi,

This is follow-up to #541 I forgot to do.

@emollier

@coveralls
Copy link

Coverage Status

coverage: 78.571% (-0.03%) from 78.599%
when pulling 46d64db on a-detiste:master
into f1b448e on G-Node:master.

@@ -105,7 +105,7 @@ def update_info(newver):
infodict = json.load(infofile)

verstring = infodict["VERSION"]
newverstring = re.sub("[0-9\.a-z]+", newver, verstring)
newverstring = re.sub(r"[0-9\.a-z]+", newver, verstring)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sligthly unrelated, but useful nonetheless, these r"" raw strings will fix a bunch of SyntaxWarnings which would stem to the failure to translate the escape code "\." in this particular case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I always run pyflakes before submitting a PR; and the warnings were there, so ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Acknowledged, I guess that explains it and the programming languages selection in setup.py. :)

@emollier
Copy link
Contributor

Thanks Alexandre for the follow-up changes to remove traces of six!

I went through your changes and I believe they are sound, but I am only a passer-by contributor to nixio/nixpy, so better wait for main upstream authors to comment.

Have a nice day, :)
Étienne.

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM. ensure_str() could be a utility function so it's not duplicated but that's minor and we can do it in a separate PR.

@achilleas-k achilleas-k merged commit 09bfbf2 into G-Node:master Nov 14, 2024
21 checks passed
@a-detiste
Copy link
Contributor Author

a-detiste commented Nov 15, 2024

We ll ... ensure_str() was there in the first place because the Py2 was not sure what is was getting. This could be removed after carefully auditing wether these are internal API with safe input or API that might get unexpected data from callers.

This kind of audit can be greatly aided by adding type annotations all over the place and running mypy.

But many people feel that type annotations doesn't belong in Python.

I may try to do the bare minimum needed to remove the two ensure_str If you are interested.

Greetings

@a-detiste
Copy link
Contributor Author

data_to_value is a copy of ensure_str with a different name.

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.

4 participants