Add license field support #78

sjlongland · 2023-09-09T02:42:42Z

One gripe I have is that tomlkit makes a new section, rather than doing this as an inline table.

So instead of this:

license = { text = "My License" }

we get:

[project.license]
text = "My License"

The latter may work better visually for multi-line licenses (the field can store a full custom license agreement if none of the "standard" ones suit), and should be equivalent in terms of TOML behaviour.

But it doesn't match the examples shown in Python packaging docs.

It's not obvious how I coax tomlkit to emit the other format.

Reading the TOML spec, the two formats are seemingly identical in terms of the data represented, so I'll leave it as-is for now. Closes Issue #32.

diazona

Sorry it took me a little while to get to this! I offered a bit of feedback, though none of the changes are strictly essential.

tests/test_setup_command.py

tests/test_license.py

src/setuptools_pyproject_migration/__init__.py

diazona · 2023-09-10T07:09:00Z

Oh I meant to reply about the formatting in the TOML output as well. It would definitely be nice if we could make it look like the Python packaging docs, but given that both forms are equivalent, I definitely don't see it as something particularly important. Maybe in the future if we find a way we can modify the code accordingly. (And if someone cares enough, they can make a manual edit to their pyproject.toml.)

diazona

I've got some stuff going on this weekend so I haven't gotten to leave a full review, but hopefully these comments help move things along 🙂

tests/test_license_files.py

tests/test_setup_command.py

tests/test_license_files.py

diazona · 2023-09-29T01:16:04Z

What's the timeline looking like for this? I think it's probably going to be the last thing blocking an initial release (or at least, among the things I added to the "initial release" milestone)

sjlongland · 2023-09-29T07:29:27Z

Yeah, this has taken way longer than I wanted. Largely because I've been busy with work commitments before I went on leave. I'm now officially on leave until 2023-11-06 so I have some time now to clean things up.

tests/test_license_files.py

diazona · 2023-10-03T16:53:33Z

LGTM now! Did you want to squash/reorganize the commits at all before this gets merged? (Just figured I'd ask because, you know, auto-merge is enabled and if I just click "approve" then it'll be too late to do any commit reorganization)

sjlongland · 2023-10-03T20:48:59Z

LGTM now! Did you want to squash/reorganize the commits at all before this gets merged? (Just figured I'd ask because, you know, auto-merge is enabled and if I just click "approve" then it'll be too late to do any commit reorganization)

I can squash it if you feel that'd be better… to be honest I find having the full commit history is useful when troubleshooting because tools like git bisect then has much finer-grained changes to work with.

If you bundle multiple changes into a single commit, git bisect starts to use its usefulness due to the coarseness of commits.

diazona · 2023-10-03T21:59:00Z

Oh for sure, I definitely agree in keeping things broken out for bisection and similar use cases. Sorry for the confusion: I didn't mean squashing everything together into a single commit, but I was thinking more like squashing some of the little commits that don't really stand on their own, like typo fixes or whatever, into the more standalone commits, just to reduce clutter in the commit history and maybe eliminate intermediate steps that don't compile when it makes sense to do so. In other words, I'm talking about an interactive rebase, not a squash-merge (which would be combining all the commits together into one). This is something I do routinely (although usually before pushing) so I do think it's a good idea, but it's not something import enough that I would try to insist on it. Totally up to you whether you want to do this or not.

I will be happy to approve either way, I'm just making sure I don't take the option away.

Nothing fancy, this text could be either a license expression (e.g. `MIT`, `GPLv2+`…) or a full-blown multi-line license string. We copy it verbatim with no interpretation.

- a simple one for the name of a license or a SPDR license expression - a more complex one that uses a real-world license (the Python software license v2) to ensure the text does not become mangled in the conversion.

…tests This is work in progress, as the underlying PEP has not yet been ratified and adopted. Once that is done, we'll re-visit these tests and amend as necessary.

sjlongland · 2023-10-04T02:03:16Z

Okay, did a final local test after a slight flattening of the commit history:

===================================== 107 passed, 4 xfailed, 2 warnings in 1980.75s (0:33:00) =====================================
.pkg: _exit> python /usr/lib/python3.11/site-packages/pyproject_api/_backend.py True setuptools.build_meta
  py: OK (2154.31=setup[172.05]+cmd[1982.26] seconds)
  congratulations :) (2154.72 seconds)

… took a bit longer than I was expecting. Hopefully CI won't take this long.

diazona · 2023-10-04T03:39:46Z

Whoa, that is a looong time.

It might be related to the distribution package testing, especially if it's the first time you're running those tests - it would have to download the source code for pytest and pytest-localserver. But still I wouldn't expect it to be that long (33 minutes!) unless you have a very slow internet connection or something like that. In any case, it looks like pre-merge tests in CI were still relatively quick in comparison.

sjlongland · 2023-10-04T03:48:13Z

Yep, I think it is Internet connection related… 25Mbps down / 2Mbps up hybrid-fibre coax.

The laptop here is no spring chicken, but it's not that slow. ps axf showed a lot of time spent running pip.

sjlongland force-pushed the feature/i32-license-field branch from abc7eaf to dcb82ee Compare September 9, 2023 02:44

sjlongland enabled auto-merge September 9, 2023 02:47

sjlongland requested a review from diazona September 9, 2023 02:48

diazona linked an issue Sep 10, 2023 that may be closed by this pull request

Support license #32

Closed

diazona requested changes Sep 10, 2023

View reviewed changes

sjlongland force-pushed the feature/i32-license-field branch from 58d1704 to 3806353 Compare September 16, 2023 22:54

diazona reviewed Sep 17, 2023

View reviewed changes

tests/test_license_files.py Outdated Show resolved Hide resolved

tests/test_license_files.py Outdated Show resolved Hide resolved

tests/test_setup_command.py Outdated Show resolved Hide resolved

tests/test_license_files.py Outdated Show resolved Hide resolved

sjlongland force-pushed the feature/i32-license-field branch 2 times, most recently from 2fe6aef to 13623e8 Compare September 29, 2023 08:07

diazona requested changes Oct 3, 2023

View reviewed changes

tests/test_license_files.py Outdated Show resolved Hide resolved

sjlongland force-pushed the feature/i32-license-field branch from 13623e8 to 56ec80e Compare October 3, 2023 07:04

sjlongland added 3 commits October 4, 2023 11:21

issue 32: __init__: Copy across the license text to project.license

f0f1eb8

Nothing fancy, this text could be either a license expression (e.g. `MIT`, `GPLv2+`…) or a full-blown multi-line license string. We copy it verbatim with no interpretation.

issue 32: license tests: Add test cases

fa7e79a

- a simple one for the name of a license or a SPDR license expression - a more complex one that uses a real-world license (the Python software license v2) to ensure the text does not become mangled in the conversion.

issue 32: license_files tests: Add to-be-implemented license_files …

8cfa924

…tests This is work in progress, as the underlying PEP has not yet been ratified and adopted. Once that is done, we'll re-visit these tests and amend as necessary.

sjlongland force-pushed the feature/i32-license-field branch from 56ec80e to 8cfa924 Compare October 4, 2023 02:03

diazona approved these changes Oct 4, 2023

View reviewed changes

sjlongland merged commit f8addca into main Oct 4, 2023

sjlongland deleted the feature/i32-license-field branch October 4, 2023 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add license field support #78

Add license field support #78

sjlongland commented Sep 9, 2023

diazona left a comment

diazona commented Sep 10, 2023

diazona left a comment

diazona commented Sep 29, 2023

sjlongland commented Sep 29, 2023

diazona commented Oct 3, 2023

sjlongland commented Oct 3, 2023

diazona commented Oct 3, 2023 •

edited

Loading

sjlongland commented Oct 4, 2023

diazona commented Oct 4, 2023

sjlongland commented Oct 4, 2023

Add license field support #78

Add license field support #78

Conversation

sjlongland commented Sep 9, 2023

diazona left a comment

Choose a reason for hiding this comment

diazona commented Sep 10, 2023

diazona left a comment

Choose a reason for hiding this comment

diazona commented Sep 29, 2023

sjlongland commented Sep 29, 2023

diazona commented Oct 3, 2023

sjlongland commented Oct 3, 2023

diazona commented Oct 3, 2023 • edited Loading

sjlongland commented Oct 4, 2023

diazona commented Oct 4, 2023

sjlongland commented Oct 4, 2023

diazona commented Oct 3, 2023 •

edited

Loading