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 missing links in INSTALL.rst and simplify language #6984

Merged

Conversation

stefanv
Copy link
Member

@stefanv stefanv commented Jun 2, 2023

No description provided.

@stefanv stefanv added the 📄 type: Documentation Updates, fixes and additions to documentation label Jun 2, 2023
@stefanv stefanv added this to the 0.22 milestone Jun 2, 2023
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Seems like an improvement. Thanks. :)

Comment on lines +9 to +12
Two popular alternatives are the pip-based
`Python.org installers <https://www.python.org/downloads/>`_
and the conda-based
`miniforge <https://github.com/conda-forge/miniforge>`_.
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
Two popular alternatives are the pip-based
`Python.org installers <https://www.python.org/downloads/>`_
and the conda-based
`miniforge <https://github.com/conda-forge/miniforge>`_.
Two popular alternatives are
`Python.org installers <https://www.python.org/downloads/>`_ which provide pip and
`miniforge <https://github.com/conda-forge/miniforge>`_ which provides conda.

This sounds "more correct" and clearer to me.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I prefer the original.

Copy link
Member

Choose a reason for hiding this comment

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

I am a bit surprised. 😄 As I'd even say that the original version is technically false. The installers themselves are not "pip-based", they make pip available. Same for miniforge.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I believe that you understand 'based' in the strictest sense. Maybe think of a 'web-based application?' It doesn't mean that the application would be made out of the web, from the web, nor with the web as its main ingredient... Rather, it means that it uses the web; stretching it a bit much (in the broadest/weakest sense), it just means that it pertains to the web.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, my gut-feeling is unchanged, but this is not a blocker for me anyway. 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm setting the stage here for the next instruction, which requires users clearly knowing which of the two installers they have available. Sometimes, instructions are clearer even when they are not technically 100% accurate :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@lagru It looks like this is still an open issue. I am going to go ahead merge it anyway, since it is a good improvement already. We should improve this further in a subsequent PR.

@stefanv stefanv force-pushed the fix-missing-installation-docs-links branch from 07ecddf to 40363b6 Compare June 6, 2023 18:10
@jarrodmillman jarrodmillman merged commit cb59104 into scikit-image:main Jun 6, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants