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

Retrieve flit's version and description by parsing the ast #333

Merged
merged 3 commits into from
Mar 24, 2021

Conversation

frostming
Copy link
Collaborator

@frostming frostming commented Mar 24, 2021

Pull Request Check List

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

Related issue pdm-project/pdm-backend#10

@frostming frostming merged commit a975156 into master Mar 24, 2021
@frostming frostming deleted the feature/flit-description branch March 24, 2021 03:35
@ntninja
Copy link
Contributor

ntninja commented Mar 24, 2021

Nit: Please emit a warning if no description could be retrieved (self._data["description"] is None). Somebody might have done from ._core import __doc__ or something (which works in flit) and they should know if PDM failed to pick it up.

@ntninja
Copy link
Contributor

ntninja commented Mar 24, 2021

Your PR doesn't account for the common idiom of from .version import __version__ in main. Many projects use (used?) this to be able to do execfile(os.path.dirname(__file__) + "/name/version.py") in their setuptools and sphinx configuration files for retrieving the just version number without importing everything. If your planning on keeping version.from then implementing this may be worthwhile since it allows to losslessly convert the version discovery logic from flit to pdm, otherwise emitting a warning when the version number is not found or found but now hard-coded into pyproject.toml would be helpful.

@frostming frostming mentioned this pull request Mar 25, 2021
2 tasks
@frostming
Copy link
Collaborator Author

frostming commented Mar 25, 2021

Static is better than dynamic, if we are to support dynamic version and description, what about __author__ and __license__ which is also a commonly used pattern. They are common before because setup.py itself is a dynamic python script where you can play many magic there, such as conditional requirements, dynamically generated README. But when we come to PEP 621, static metadata is recommended, though dynamic fields are not obsoleted fully(they can appear in dynamic). So let's only keep the from.version which is from pdm 0.x versions.

I have submitted a PR to emit warnings when those fields can't be determined when importing from a flit pyproject.toml, give it a try and feedbacks are welcome.

@ntninja
Copy link
Contributor

ntninja commented Mar 26, 2021

The warning LGTM, but in case where conversion succeeds a short notice about the values now being managed in pyproject.toml rather than the Python file would also be helpful to new users, I think.

Also it copying the version number from __version__ into PEP-621 data, rather then creating a from.version reference, is done on purpose to encourage the use of static metadata?
If so, it would be nice if the notice also included a link to https://pdm.fming.dev/pyproject/#determine-the-package-version-dynamically for people preferring to keep the previous behaviour for that field at least.

(Saying this as somebody who thought the way it previously converted the version was a bug, particularly after I discovered how it's actually possible to keep the existing behaviour there.)

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.

2 participants