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

Feature/unify readme #186

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

rachmadaniHaryono
Copy link

@rachmadaniHaryono rachmadaniHaryono commented Jun 28, 2018

fix #171

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I've posted a few advices for you on how to improve things + fix CI.

Please also take into account this suggestion #171 (comment)

@rachmadaniHaryono
Copy link
Author

rachmadaniHaryono commented Jun 29, 2018

hi, i just made the edit as you suggested

i left the original installation instruction, to let user know the different way to install it

e: just let me know if it should be deleted

Copy link

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

LGTM

@webknjaz
Copy link

webknjaz commented Jul 3, 2018

@ingydotnet this PR is ready, please merge it before next release as it will dramatically improve how PYPI page looks.

@ingydotnet
Copy link
Member

@webknjaz will do for the 4.2 release. Cheers.

@webknjaz
Copy link

webknjaz commented Nov 4, 2018

@ingydotnet bump

@bsolomon1124
Copy link
Contributor

FYI that as of pip 20.x, pip install pyyaml --install-option=--without-libyaml errors out ("error: option --without-libyaml not recognized") whereas --global-option=--without-libyaml succeeds.

I'm not sure why, but suspect it has to do with some outdated stuff in setup.py that pip does not respect.

@rachmadaniHaryono
Copy link
Author

rachmadaniHaryono commented May 28, 2020

maybe this is the cause

Deprecate passing install-location-related options via --install-option.

https://pip.pypa.io/en/stable/news/#id50

pypa/pip#7309
i will change the pr and add the edit as @bsolomon1124 suggested this week

e: done.

i kept the install-option doc for pip < 20.0

e2: wrong guess

setup.cfg Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@nitzmahone
Copy link
Member

Yeah, that's one of the reasons I've not tried to attack a full setuptools cutover or any more ambitious refactoring of the packaging/install stuff- we either need to kill all the ancient custom arg stuff and lean more fully on the standard knobs provided by modern packaging infra for extension build control, or figure out sane ways to do it on setuptools.

Relevant: https://twitter.com/codewithanthony/status/1265697485780955137?s=19 ;)

@bsolomon1124
Copy link
Contributor

bsolomon1124 commented May 28, 2020

@nitzmahone

Yeah, that's one of the reasons I've not tried to attack a full setuptools cutover or any more ambitious refactoring of the packaging/install stuff

I don't blame you, especially since PEP 517 is still provisional.

I assume you're alluding to the --install-option issue with that comment? My two cents is that a complete rewrite of setup.py (or move towards pyproject.toml altogether) to address that and drop the distutils/sys.modules black magic is just not worth it right now. There are a lot of open PRs and issues (this one being a good example) that are already made-to-order and produce a positive change to pyyaml as-is.

@nitzmahone
Copy link
Member

nitzmahone commented May 28, 2020

Yeah, I'm actually reasonably confident that absent any legacy concerns, we could have a nice clean modern setup experience, but that screws everyone eg, running older stock Ubuntu LTS releases with old bundled pip (and other things) that would just fail completely (or worse, in subtly weird ways).

I feel like this project has a large accumulation of tech debt- most of the code and layout is ancient and creaky, but still works quite well for what it does. My interest and involvement in it is purely selfish and mostly defensive ("don't break Ansible"), so I have a hard time getting excited about big projects or sweeping changes that will destabilize it for myself and others, especially if the time taken to fix/restabilize those things detracts from my primary project...

@rachmadaniHaryono
Copy link
Author

Should probably confirm the specific version of pip where the --install-option versus --global-option changed. I'm not certain if it's pip 19, pip 20, or something else.

Edit: I think this is because the custom Distribution in setup.py uses global_options.

i just tested with pip 19.0 and the same error happened, so it mean my guess that it is caused by pip 20.0 is wrong i will remove install-option and use global-option only.

Summary of documentation-related issues that can probably be addressed at once:

i just noticed about #327. if pyyaml dev want to use markdown than rst, i will remove rst related pr on this and contribute to 327.

this will remove the README.rst and move the content to README.md

this include

- badge
- recommended installation method
- recommended way for alternative method
- fix link to document on above point
- code markup

it also point to README.md for LONG_DESCRIPTION

Signed-off-by: Rachmadani Haryono <foreturiga@gmail.com>
@rachmadaniHaryono
Copy link
Author

at the end i use markdown and move all the change from readme.rst to main readme.md

the change is simpler than before

reference on setup.py change https://packaging.python.org/guides/making-a-pypi-friendly-readme/#including-your-readme-in-your-package-s-metadata

on that page the example use pathlib but i still use codecs as suggested above

i also notice this

Merging is blocked
The base branch requires all commits to be signed

if required i will close the pr and create a new one

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.

Unify README with PYPI
5 participants