Opened 2 years ago

Closed 2 years ago

#30630 closed enhancement (fixed)

findstat error handling

Reported by: mantepse Owned by:
Priority: minor Milestone: sage-9.3
Component: combinatorics Keywords: findstat
Cc: Merged in:
Authors: Martin Rubey Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: 3f89b5b (Commits, GitHub, GitLab) Commit: 3f89b5b6ca0e3fe607ca1dc6028c9b137777e006
Dependencies: Stopgaps:

Status badges

Description (last modified by mantepse)

Raise appropriate errors when there are problems in the communication with findstat.org.

Moreover, add support for the new findstat collection "finite lattices".

Finally, we fix some pyflakes issues.

Change History (18)

comment:1 Changed 2 years ago by mantepse

Branch: u/mantepse/findstat_error_handling

comment:2 Changed 2 years ago by git

Commit: 6d311b2c6c988b1f258d6ac5c080231280c47ae9

Branch pushed to git repo; I updated commit sha1. New commits:

6d311b2add doctest

comment:3 Changed 2 years ago by mantepse

Authors: Martin Rubey
Component: PLEASE CHANGEcombinatorics
Description: modified (diff)
Keywords: findstat added
Priority: majorminor
Type: PLEASE CHANGEenhancement

comment:4 Changed 2 years ago by slelievre

Use an url at example.com so nobody can play a trick? Check if that works.

-        sage: _get_json("https://nonexistingfindstaturl.org")
+        sage: _get_json("https://not-findstat.example.com")
         Traceback (most recent call last):
         ...
-        ConnectionError: HTTPSConnectionPool(host='nonexistingfindstaturl.org', port=443): Max retries exceeded with url: / (Caused by NewConnectionError('...Failed to establish a new connection: [Errno -2] Name or service not known'))
+        ConnectionError: HTTPSConnectionPool(host='not-findstat.example.com', port=443): Max retries exceeded with url: / (Caused by NewConnectionError('...Failed to establish a new connection: [Errno -2] Name or service not known'))

Don't forget to set to needs_review when ready.

comment:5 Changed 2 years ago by git

Commit: 6d311b2c6c988b1f258d6ac5c080231280c47ae98ab688cc7b9705e12fe203ffd25f4d71fae233e8

Branch pushed to git repo; I updated commit sha1. New commits:

8ab688cbetter error handling and doctest

comment:6 Changed 2 years ago by mantepse

Status: newneeds_review

It turns out that testing what I really want to test is tricky:

  • I originally wanted to test that an error is raised when findstat is down.
  • using badurl.example.com or the like does not test this, because the domain does not exist. In this case request also throws nested errors, which are hard to catch.

Therefore, I test something else, which makes more sense to test anyway.

comment:7 Changed 2 years ago by git

Commit: 8ab688cc7b9705e12fe203ffd25f4d71fae233e812f0339fccc666adf6b41632a15f5fc1bd998367

Branch pushed to git repo; I updated commit sha1. New commits:

12f0339error handling also for post requests

comment:8 Changed 2 years ago by git

Commit: 12f0339fccc666adf6b41632a15f5fc1bd998367a26e753b94ff0f1019225b10fe3a24b63ddeb08e

Branch pushed to git repo; I updated commit sha1. New commits:

a26e753reenable support for unsupported collections, remove obsolete code

comment:9 Changed 2 years ago by git

Commit: a26e753b94ff0f1019225b10fe3a24b63ddeb08e27476c54e24b974027cc85c2325a0ebc1d6506bb

Branch pushed to git repo; I updated commit sha1. New commits:

27476c5add lattices to supported collections

comment:10 Changed 2 years ago by git

Commit: 27476c54e24b974027cc85c2325a0ebc1d6506bb3f89b5b6ca0e3fe607ca1dc6028c9b137777e006

Branch pushed to git repo; I updated commit sha1. New commits:

3f89b5bfix doctests, simplify iterator, fix missing import

comment:11 Changed 2 years ago by gh-kliem

Can you please modify the ticket description so that you see that the support for new collections is added.

comment:12 Changed 2 years ago by mantepse

Description: modified (diff)

comment:13 Changed 2 years ago by vdelecroix

Why are you turning the JSONDecodeError into a ValueError?

comment:14 Changed 2 years ago by mantepse

Because the reason for the error is (most likely) an inappropriate value response.text, due to (most likely) a bad url or a new but on the findstat server. I felt that a ValueError would then be more correct.

For example, upon

sage: findstat()
The Combinatorial Statistic Finder (https://www.findstat.org/)
sage: sage.databases.findstat._get_json("https://www.findstat.org/")

if I only threw the JSONDecodeError, the message would simply be

JSONDecodeError: Expecting value: line 1 column 1 (char 0)

which wouldn't be helpful at all.

Thus I want to provide the response.text, which in this example would be the html of the frontpage of findstat. Seeing this, I would immediately know that I forgot api in the url, which is a ValueError, I think.

comment:15 Changed 2 years ago by slabbe

Reviewers: Sébastien Labbé
Status: needs_reviewpositive_review

green bot + works for me

comment:16 Changed 2 years ago by mantepse

Thank you!

comment:17 Changed 2 years ago by mkoeppe

Milestone: sage-9.2sage-9.3

comment:18 Changed 2 years ago by vbraun

Branch: u/mantepse/findstat_error_handling3f89b5b6ca0e3fe607ca1dc6028c9b137777e006
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.