Skip to content

Formally deprecate distro.linux_distribution()#296

Merged
jdufresne merged 1 commit intopython-distro:masterfrom
jdufresne:emit-warning
Jul 8, 2021
Merged

Formally deprecate distro.linux_distribution()#296
jdufresne merged 1 commit intopython-distro:masterfrom
jdufresne:emit-warning

Conversation

@jdufresne
Copy link
Copy Markdown
Member

Users should use distro.id(), distro.version(), and distro.name()
instead.

Refs #263

@HorlogeSkynet
Copy link
Copy Markdown
Member

There is a typo : s/compatability/compatibility/.

Copy link
Copy Markdown
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

Looks good, one nit:

Comment thread distro.py Outdated
Users should use distro.id(), distro.version(), and distro.name()
instead.

Refs #263
@jdufresne
Copy link
Copy Markdown
Member Author

Thanks for the reviews. I addressed all feedback in the latest revision.

Copy link
Copy Markdown
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM, pinging @nir0s as this is a big manifesto change. After this should we remove mentions to distro.linux_distribution() in the README, repository description, etc

@HorlogeSkynet
Copy link
Copy Markdown
Member

Agreeing with @sethmlarson here. From my PoV this project (first) intended to be a drop-in replacement for the platform deprecated method. Maybe the official Python documentation should be updated too, as it points to this module for this specific purpose ?

@jdufresne
Copy link
Copy Markdown
Member Author

I'll hold off on merging until @nir0s has a chance to weigh in.

@nir0s
Copy link
Copy Markdown
Collaborator

nir0s commented Jul 7, 2021

Guys, I appreciate you holding off, but let's not make a habit out of it. If you feel like there's a huge change, and you want me to pitch in, I'll try. However, if you think that it makes sense to do something, and I'm unresponsive, go ahead and do it.

Anyway, to address the comments:

LGTM, pinging @nir0s as this is a big manifesto change. After this should we remove mentions to distro.linux_distribution() in the README, repository description, etc

Indeed we should.

with @sethmlarson here. From my PoV this project (first) intended to be a drop-in replacement for the platform deprecated method. Maybe the official Python documentation should be updated too, as it points to this module for this specific purpose?

It's only my opinion, but I see no reason why there's a need for a single function that returns a tuple with an unclear structure. What's the first part? Is it the full name? The same questions apply to the codename and the version. Given that explicit is better than implicit, I'd rather do something like:

if distro.id() == "..." and distro.name() == "...":
   ...

which also allows me to pass fields explicitly to the relevant function (e.g. best),

Instead of:

distro_info = distro.linux_distribution()

if distro_info[0] == "you get the point":
  ...

I indeed think that the official Python documentation should be updated, and I don't mind deprecating this until distro v2.0 comes out, if only to give people a shitload of time to stop using it.

Honestly, I'm not strongly opinionated about this. distro is supposed to provide a reasonable alternative to an annoying problem. If the Python community responds badly to removing linux_distribution, AFAIC, we can leave it be. It's not like it takes a lot of effort to maintain.

However, even if we do decide to keep it, we can push to update the official Python docs to encourage people to use distro.id(), etc.. instead.

Thoughts?

@jdufresne
Copy link
Copy Markdown
Member Author

Thanks.

I indeed think that the official Python documentation should be updated, and I don't mind deprecating this until distro v2.0 comes out, if only to give people a shitload of time to stop using it.

This makes sense. The warning is a good start and there is no rush to remove or anything like that.

@jdufresne jdufresne merged commit f947776 into python-distro:master Jul 8, 2021
@jdufresne jdufresne deleted the emit-warning branch July 8, 2021 03:31
@hartwork hartwork added this to the 1.6.0 milestone Jul 10, 2021
@HorlogeSkynet
Copy link
Copy Markdown
Member

HorlogeSkynet commented Jul 30, 2021

@python-distro/maintainers

Dear maintainers, I guess we should update the GitHub repository description now that this deprecation has been released.

Any idea ? What about :

- A much more elaborate, renewed alternative implementation for Python's platform.linux_distribution()
+ A much more elaborate replacement for removed Python's `platform.linux_distribution()` method

Thanks 🙏


EDIT 2021-07-31 :

  • description updated with proposed candidate
  • python-library (recommended) tag added
  • HTTPS URL scheme preferred for documentation link

bmw added a commit to certbot/certbot that referenced this pull request Aug 2, 2021
`distro.linux_distribution` was deprecated (python-distro/distro#296) in the release of `distro` at the end of last week. The deprecation is causing the `nopin` nightly tests to fail.

This change migrates Certbot off that function.

As far as I can tell, the Arch Linux edge case described in the code comments no longer happens, but better to be safe than sorry I think.

* stop using deprecated distro.linux_distribution

* update comment

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
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.

5 participants