aiidateam / qe-tools

A set of useful tools for Quantum ESPRESSO
MIT License
28 stars 13 forks source link

Drop py2 support? #31

Closed greschd closed 4 years ago

greschd commented 4 years ago

Since the develop version of aiida-quantumespresso has just dropped py2 support, does it make sense to do the same here?

The reason I'm asking is because the interface of the cell -> parameters function I'm working on would benefit from keyword-only arguments.

giovannipizzi commented 4 years ago

I think it's ok to make a version dropping py2 support. I won't have much time, if you want to work on it (I don't think there is too much work) and make a PR, that would be great!

As I mentioned earlier, I would use the excuse to release a new major release, fixing a thing that is bugging me, i.e. the interface of the __init__ of the parser: https://github.com/aiidateam/qe-tools/blob/c9eac45b28a54b630ef148290b1ccc838b5c3abe/qe_tools/parsers/qeinputparser.py#L149-L155

In particular I find it confusing that if the string is a valid abs path, it is interpreted as the file name, otherwise as the content (and e.g. if I pass a relative path, the second route is taken).

I would suggest that if it is a string, it's the file content, otherwise if it's a file-like object, it should be already open, and the function just calls .open()? Or we just go with the file content as a string (it's the input of QE, so it should never be huge)?

sphuber commented 4 years ago

I have been using the following sed commands to drop the big majority of py2 compatibility code:

find . -type f -not -path './.git*' -exec sed -i '/from __future__.*$/d' {} +
find . -type f -not -path './.git*' -exec sed -i 's/(object):/:/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/super([^)]*,[^)]*)/super()/g' {} +
find . -type f -not -path './.git*' -exec sed -i '/\(import six\|from six\).*$/d' {} +
find . -type f -not -path './.git*' -exec sed -i '/\(six\.add_metaclass\).*$/d' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.integer_types/int/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.string_types/str/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.text_type/str/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.binary_type/bytes/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.moves.StringIO/io.StringIO/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.StringIO/io.StringIO/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.moves.BytesIO/io.BytesIO/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.BytesIO/io.BytesIO/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's/six.moves.range/range/g' {} +
find . -type f -not -path './.git*' -exec sed -i 's|six.iterkeys(\([^)]*\))|\1.keys()|' {} +
find . -type f -not -path './.git*' -exec sed -i 's|six.iteritems(\([^)]*\))|\1.items()|' {} +
find . -type f -not -path './.git*' -exec sed -i 's|six.itervalues(\([^)]*\))|\1.values()|' {} +
greschd commented 4 years ago

Re: the the __init__ interface:

I'm all in favour of making that interface a bit more consistent. To be honest, I'm not even sure all these different cases need to be handled here - I would also be fine with an interface that only takes the string variant, for example. The steps needed to convert it (e.g. open() and .read(), or ''.join(..)) are so simple that I'd be tempted to say this can go in the calling code.

giovannipizzi commented 4 years ago

Agreed. Let's go with the string variant only then!

giovannipizzi commented 4 years ago

Thanks @greschd ! Should we move the discussion of the init in a different thread, or you are going to make a PR for that as well soon? Also, can you already upgrade the version number to a new major version when this is done, so we don't forget? Maybe an alpha 1 version?

greschd commented 4 years ago

We can keep it here, since we've already come to a conclusion.

Do you mind if I also fiddle around with the scaffolding (tests, pre-commit, etc.) a bit?

giovannipizzi commented 4 years ago

You are more than welcome!!

giovannipizzi commented 4 years ago

BTW, I approved #33, feel free to merge - and feel free to make any scaffolding changes or other that you want.

Also, once this will be ready, I'm happy to give you access to pypi if you want to be able to make releases

greschd commented 4 years ago

Also, once this will be ready, I'm happy to give you access to pypi if you want to be able to make releases

Thanks! My PyPI username is greschd, same as on GitHub.

Since we're slightly breaking backwards compatibility with the next release, is there any other downstream code (that you're aware of) other than aiida-quantumespresso?

giovannipizzi commented 4 years ago

Added you on Pypi as maintainer! There is tools-barebone that I am updating these days, and seekpath (that I am converting to tools-barebone, so it does not matter so much). Not sure about others (but if we make a major release, we should be OK).