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

Fix requirement files and package data #2156

Merged
merged 3 commits into from Apr 26, 2023
Merged

Conversation

DanielNoord
Copy link
Collaborator

Type of Changes

Type
✨ New feature
🔨 Refactoring

Description

Closes #2129

The first commit has been a pet peeve of mine for months. I really disliked how it was hard to see what you need to install when first opening the project. I hope this lay out is a bit more explicit.

The second commit adds the files as requested by @mtelka.
Tested with pip install . and the files ended up in the egg info. @mtelka could you test if this branch fits your needs?

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Apr 26, 2023
@DanielNoord DanielNoord added this to the 3.0.0a3 milestone Apr 26, 2023
@DanielNoord DanielNoord changed the title Reqs Fix requirement files and package data Apr 26, 2023
@DanielNoord
Copy link
Collaborator Author

Sorry for the spam guys. I'm walking to the coffee machine as we speak to hopefully prevent more commits 😄

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM

# Tools used during development, prefer installing these with pre-commit
black
pre-commit
pylint
Copy link
Member

Choose a reason for hiding this comment

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

pylint cannot be installed with pre-commt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

"prefer installing these with pre-commit", this is not possible for pylint as it's a system hook.

@@ -0,0 +1,8 @@
# Tools used when releasing
contributors-txt>=0.7.4
tbump~=6.9.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tbump~=6.9.0
tbump~=6.9

@mtelka
Copy link

mtelka commented Apr 26, 2023

I reviewed changes and they looks reasonable, but I'm not sure how to test this properly without a sdist release. Anyway, my workflow (packaging of Python projects for OpenIndiana) does not require perfect sdists because I can easily patch them (as I currently do with astroid 2.15.4), so if there will be something still not working as expected I'll report back.

Thank you!

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #2156 (2d350dd) into main (42f261d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2156   +/-   ##
=======================================
  Coverage   92.54%   92.54%           
=======================================
  Files          94       94           
  Lines       10783    10783           
=======================================
  Hits         9979     9979           
  Misses        804      804           
Flag Coverage Δ
linux 92.30% <ø> (ø)
pypy 87.48% <ø> (ø)
windows 92.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@DanielNoord DanielNoord merged commit da24639 into pylint-dev:main Apr 26, 2023
18 checks passed
@DanielNoord DanielNoord deleted the reqs branch April 26, 2023 10:42
@hroncok
Copy link
Contributor

hroncok commented Jun 29, 2023

This makes the files installed.

(venv) [tmp]$ pip install https://github.com/pylint-dev/astroid/archive/da246398cb42c1c0509ce572cfb93b6bc1776c1a.zip
...

(venv) [tmp]$ ls -1 venv/lib/python3.11/site-packages/
...
astroid
astroid-3.0.0a3.dev0.dist-info
...
requirements_dev.txt
requirements_full.txt
requirements_minimal.txt
...
tests
tox.ini

@DanielNoord
Copy link
Collaborator Author

@hroncok I always mess this up. How should we do this to prevent polluting the global namespace?

@hroncok
Copy link
Contributor

hroncok commented Jun 29, 2023

Don't package-data it, include it in the sdist only? Back in the day, that was done using MANIFEST.in. No idea if it's possible to do it via pyproject.toml.

@DanielNoord
Copy link
Collaborator Author

@hroncok Sorry for taking so long to respond. This has been constantly on my radar, but I really have no idea how to fix this. If I include the files in MANIFEST.in and remove them from the pyproject.toml I still get them when doing pip install .. Is that expected? I'm completely lost here.

@hroncok
Copy link
Contributor

hroncok commented Sep 14, 2023

If I include the files in MANIFEST.in and remove them from the pyproject.toml I still get them when doing pip install .. Is that expected?

No, that's not expected at all. And I cannot reproduce it. See #2295 which works for me.

@DanielNoord
Copy link
Collaborator Author

Hmm that did seem to work. No idea why it didn't on initial try locally. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining astroid or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sdist is missing tox.ini and resources.py
4 participants