Opened 3 years ago

Closed 3 years ago

#27748 closed enhancement (fixed)

avoid using is_package_installed in generic_graph.py

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.8
Component: graph theory Keywords:
Cc: fbissey, dimpase Merged in:
Authors: Jeroen Demeyer Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: d3904e2 (Commits, GitHub, GitLab) Commit: d3904e2fb4e4a3c0e60b157d303356d0c168ddd0
Dependencies: Stopgaps:

Status badges

Description

Follow up to #27628. See #27628#comment:11

Change History (13)

comment:1 follow-up: Changed 3 years ago by dcoudert

I opened this ticket but we must wait for 8.8.beta4.

Note that I find is_package_installed much more natural than a try:... except... here, but if this method is not good, let's use something else.

comment:2 in reply to: ↑ 1 Changed 3 years ago by fbissey

Replying to dcoudert:

I opened this ticket but we must wait for 8.8.beta4.

Note that I find is_package_installed much more natural than a try:... except... here, but if this method is not good, let's use something else.

Thanks for doing that. I should have done it myself and indeed we need to wait for the next beta.

There is nothing natural about is_package_installed. Which piece of software can you name that will call rpm or apt to figure out if some of their dependencies are installed? How portable do you think that is? try:... except... is a well accepted pythonism in other python software that don't have their own package manager to interrogate.

Last edited 3 years ago by fbissey (previous) (diff)

comment:3 follow-up: Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

I'm not even convinced that we should use PackageNotFoundError, just let the import fail. François, do you have an opinion on that?

comment:4 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/avoid_using_is_package_installed_in_generic_graph_py

comment:5 Changed 3 years ago by jdemeyer

  • Commit set to 6764b7604c70f15cbe46cfe73322fcc9e413138e
  • Status changed from new to needs_review

New commits:

8ee4a7atrac #27628: doctest with igraph
e6051f5trac #27628: Merged with 8.8.beta3
1e773f5trac #27628: fix pyflakes issue with igraph
6764b76Avoid is_package_installed

comment:6 in reply to: ↑ 3 ; follow-up: Changed 3 years ago by fbissey

Replying to jdemeyer:

I'm not even convinced that we should use PackageNotFoundError, just let the import fail. François, do you have an opinion on that?

Long term we will probably want to drop it as well. I am not so sure about just letting it fail or letting it print a message about missing dependency. I think that would be the first place where we just let it fail. I would have thought best practise was to inform of the missing dependency. But if you tell me otherwise I won't really object.

comment:7 Changed 3 years ago by dcoudert

  • Reviewers set to David Coudert

there is an error: it is vertex_list and not vertexlist in v_to_int = {v: i for i, v in enumerate(vertexlist)}.

We could also change vertex_list = self.vertices() to vertex_list = list(self), but it is documented like that. So may be in a future ticket.

comment:8 Changed 3 years ago by git

  • Commit changed from 6764b7604c70f15cbe46cfe73322fcc9e413138e to d3904e2fb4e4a3c0e60b157d303356d0c168ddd0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d3904e2Avoid is_package_installed

comment:9 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to fbissey:

Long term we will probably want to drop it as well. I am not so sure about just letting it fail or letting it print a message about missing dependency. I think that would be the first place where we just let it fail. I would have thought best practise was to inform of the missing dependency. But if you tell me otherwise I won't really object.

I use the Feature now which gives a nice error message.

comment:10 follow-up: Changed 3 years ago by dcoudert

I like it !

Could you add a doctest in igraph_feature (the patchbot complain that doctest coverage is reduced).

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by fbissey

Replying to dcoudert:

I like it !

Could you add a doctest in igraph_feature (the patchbot complain that doctest coverage is reduced).

Feels overkill for test which will probably have to be marked as random.

comment:12 in reply to: ↑ 11 Changed 3 years ago by dcoudert

  • Status changed from needs_review to positive_review

Feels overkill for test which will probably have to be marked as random.

It's only to avoid doctest coverage regression, but I agree it's overkill.

So, let's go like that.

comment:13 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/avoid_using_is_package_installed_in_generic_graph_py to d3904e2fb4e4a3c0e60b157d303356d0c168ddd0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.