Test with tomli

[fridex]

This introduces a breaking change

• No

Description

Check if micropipenv is compatible with tomli.

[sesheta]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from fridex after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fridex]

/hold

[frenzymadness]

I've tried to reproduce the error and it seems that the unencrypted git:// protocol is no longer supported, see https://github.blog/2021-09-01-improving-git-protocol-security-github/
We should change all git:// to https://. I'm running the tests locally for one of the new environments with this change applied and I'll see what happens.

[frenzymadness]

And the error is: module 'tomli' has no attribute 'TomlDecodeError'. It seems that the name in tomli is different (TOMLDecodeError): https://github.com/hukkin/tomli/blob/5701c0be3a76322372f58af0e19163037fec7663/src/tomli/_parser.py#L49

[frenzymadness]

We can handle this in _import_toml or we can open an upstream issue - it might make sense to create an alias to have the same exception name in these backends. What do you think?

[fridex]

Thanks for checking this! 👍🏻

We can handle this in _import_toml or we can open an upstream issue - it might make sense to create an alias to have the same exception name in these backends. What do you think?

+1, might be a good idea. Let's see what's the upstream opinion on this.

[frenzymadness]

Upstream issue: hukkin/tomli#167

[frenzymadness]

The issue has been closed. We have to handle this here. Do you wanna do it or should I take a look?

[fridex]

The issue has been closed. We have to handle this here. Do you wanna do it or should I take a look?

Oh, that's a pity. It might be good to have it eventually backward compatible with (older) tomli releases anyway if tomli support will be considered. Feel free to take it if you want 👍🏻

BTW there are also other libraries that manipulate with TOML besides pytoml and toml which micropipenv currently supports - rtoml, pytomlpp, qtoml, tomlkit (from tomli docs). It looks like tomli is now vendored in pip, hence this testing PR. I'm unsure which TOML parsers we should support. What is your opinion on this?

[frenzymadness]

During working on this, I've discovered that our support for pytoml is not complete :( The problem is that we do not have any negative tests for this

micropipenv/micropipenv.py

Lines 239 to 245 in d0c37c1

	try:
	with open(pipfile_path) as input_file:
	return toml.load(input_file)
	except toml.TomlDecodeError as exc:
	raise FileReadError("Failed to parse Pipfile: {}".format(str(exc))) from exc
	except Exception as exc:
	raise FileReadError(str(exc)) from exc

What I mean is that pytoml also does not have TomlDecodeError but we don't know about it because the except clause is not evaluated until an exception is raised in the try block.

[frenzymadness]

Take a look at my implementation. I'm not sure why some tests failed. Will try to run the same envs locally.

[frenzymadness]

It seems to me that the very old pip does not work well with the latest setuptools so I'll have to find a way how to pin setuptools to some older version which won't be an easy task. I don't want to create a new variable for that. I'd rather somehow merge that information to one variable specifying all the dependencies at once.

[frenzymadness]

It seems that setuptools<40 is too old for Python 3.9, testing again with <60.

[frenzymadness]

Almost all green. pre-commit is complaining about trailing whitespace in a file we haven't touched here but I'll fix it in the next commit to make it happy.

[frenzymadness]

@fridex I cannot request a review from you because you are the author of this PR so I've assigned it to you.

[frenzymadness]

BTW there are also other libraries that manipulate with TOML besides pytoml and toml which micropipenv currently supports - rtoml, pytomlpp, qtoml, tomlkit (from tomli docs). It looks like tomli is now vendored in pip, hence this testing PR. I'm unsure which TOML parsers we should support. What is your opinion on this?

To answer your question – Having toml is a good idea because it's defacto standard now and we have it in Fedora. pytoml is important for us (even it's an abandoned project) because we have it in RHEL 8. And finally, tomli is a good idea to add because there is some probability that it'll be a part of the standard library soon, see: hukkin/tomli#141

[fridex]

This looks very nice! Thanks for taking care of this. 👍🏻 LGTM

[fridex]

@frenzymadness I'm happy to have this merged if you are fine with that.