Opened 10 months ago

Closed 6 months ago

#27562 closed task (fixed)

Fix string formatting in ValueError in sql_db.py

Reported by: slelievre Owned by:
Priority: minor Milestone: sage-8.9
Component: packages: optional Keywords: beginner
Cc: Merged in:
Authors: Beatriz Galiana Carballido Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 8c63326 (Commits) Commit: 8c633267e64dc4643b5efd7b95cba86a4d0e174e
Dependencies: Stopgaps:

Description (last modified by slelievre)

In src/sage/databases/sql_db.py, lines 465--467 currently read

            if query_dict['table_name'] not in skel:
                raise ValueError("Database has no table" \
                    + str(query_dict['table_name']) + ".")

but should instead read

            if query_dict['table_name'] not in skel:
                raise ValueError("database has no table %s"
                                 % query_dict['table_name'])

The current version in particular lacks a space between "has no table" and the name, so errors look like:

File ...
Failed example:
    r = SQLQuery(DB, {'table_name':'simon'})
Exception raised:
...
ValueError: Database has no tablesimon.

instead of

File ...
Failed example:
    r = SQLQuery(DB, {'table_name':'simon'})
Exception raised:
...
ValueError: database has no table simon

At line 1400, one should also change

        if table_name in self.__skeleton__:
            raise ValueError('Database already has a table named' \
                + '%s.'%table_name)

to

        if table_name in self.__skeleton__:
            raise ValueError('database already has a table named %s'
                             %table_name)

Change History (25)

comment:1 Changed 10 months ago by slelievre

  • Component changed from PLEASE CHANGE to packages: optional
  • Description modified (diff)
  • Keywords beginner added
  • Priority changed from major to minor
  • Reporter changed from gh-abhayptp to slelievre
  • Summary changed from Fixes cycle basis for multiedges to Fix string formatting in ValueError in sql_db.py
  • Type changed from PLEASE CHANGE to task

Note: re-using this ticket created by mistake; the one about cycle basis for multiedges is at #27563.

comment:2 follow-up: Changed 10 months ago by slelievre

  • Description modified (diff)

This should be an easy ticket to fix for a beginner.

comment:3 Changed 10 months ago by slelievre

  • Description modified (diff)

comment:4 Changed 10 months ago by gh-beagaliana

Last edited 10 months ago by gh-beagaliana (previous) (diff)

comment:5 in reply to: ↑ 2 Changed 10 months ago by gh-beagaliana

Replying to slelievre:

This should be an easy ticket to fix for a beginner.

As a beginner I'd like to fix this, if no one else's on it, of course. Is that okay?

Last edited 10 months ago by gh-beagaliana (previous) (diff)

comment:6 Changed 10 months ago by slelievre

Sure, please go ahead. Ask questions if you have any doubt.

comment:7 Changed 10 months ago by gh-beagaliana

  • Branch set to u/gh-beagaliana/fix_string_formatting_in_valueerror_in_sql_db_py

comment:8 Changed 10 months ago by gh-beagaliana

  • Authors set to Beatriz Galiana Carballido
  • Commit set to a27f054ecc618bb2622f06c67fc3e416db504987
  • Status changed from new to needs_review

New commits:

a27f054Fix string formatting in ValueError in sql_db.py

comment:9 follow-up: Changed 10 months ago by chapoton

please add a doctest to check your new error message

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 months ago by gh-beagaliana

Replying to chapoton:

please add a doctest to check your new error message

I've never added a doctest before so this question might be a bit too simple. I've been reading the general conventions about writing tests but I'm not really sure about where should I add it. I thought I would just write it as a comment starting with something like this

Test that :trac:27562 is fixed::

and then just add the test itself.

Thank you!

Bea

comment:11 in reply to: ↑ 10 Changed 9 months ago by gh-beagaliana

I already got my answer, I'll try to add the doctest asap :-)

Last edited 9 months ago by gh-beagaliana (previous) (diff)

comment:12 Changed 9 months ago by git

  • Commit changed from a27f054ecc618bb2622f06c67fc3e416db504987 to b70fe5641ae31f82f2678018b399c3be5e58d87f

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

b70fe56Add doctest

comment:13 follow-up: Changed 9 months ago by chapoton

ok, almost good.

  • This added piece of doc should not be in the middle of the code, but with the rest of the doc (probably somewhere slightly above the place where you have put it)
  • There should be an empty line after the line ending with ::
  • The test lines after any :: should be indented by 4 more spaces
  • The correct syntax for the trac role is :trac:`27562`

comment:14 Changed 9 months ago by git

  • Commit changed from b70fe5641ae31f82f2678018b399c3be5e58d87f to 7a678a28fd8a071c6a9702d07bb189779b0389ff

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

7a678a2Update doctest

comment:15 in reply to: ↑ 13 Changed 9 months ago by gh-beagaliana

Replying to chapoton:

ok, almost good.

  • This added piece of doc should not be in the middle of the code, but with the rest of the doc (probably somewhere slightly above the place where you have put it)
  • There should be an empty line after the line ending with ::
  • The test lines after any :: should be indented by 4 more spaces
  • The correct syntax for the trac role is :trac:`27562`

Thank you for your corrections! I just changed the doctest, I believe I did everything you suggested so it should be okay by now

Last edited 9 months ago by gh-beagaliana (previous) (diff)

comment:16 Changed 9 months ago by chapoton

Looks better,

  • you forgot to remove the "doc in the middle of the code"
  • just have one (not two) empty line before and after the new piece of doc

comment:17 Changed 9 months ago by git

  • Commit changed from 7a678a28fd8a071c6a9702d07bb189779b0389ff to 01b0349996fcec3c295bf16e02195e971408b435

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

01b0349Fix typo in doctest

comment:18 Changed 7 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

comment:19 Changed 7 months ago by gh-NebulousOinkler

  • Branch changed from u/gh-beagaliana/fix_string_formatting_in_valueerror_in_sql_db_py to u/gh-NebulousOinkler/fix_string_formatting_in_valueerror_in_sql_db_py

comment:20 Changed 7 months ago by gh-NebulousOinkler

  • Commit changed from 01b0349996fcec3c295bf16e02195e971408b435 to a092f680c401bf37778f2de00d373e895bee7643

Minor doctest formatting fixes made. Also made a change to the string formatting of ValueError? on line 489 exactly analogous to the ticket.


New commits:

bb0d767Fixed doctests for #27562
a092f68Removed ellipses in doctests for #27562 and minor fix to string formatting in line 489

comment:21 follow-up: Changed 7 months ago by chapoton

The removal of ellipses looks strange. Any over traceback in doctests is using ...

Maybe they were just wrongly indented ?

comment:22 in reply to: ↑ 21 Changed 7 months ago by gh-NebulousOinkler

Replying to chapoton:

Maybe they were just wrongly indented ?

Yes, that was an issue. I'll return the ellipses to be in line with other doctests.

comment:23 Changed 7 months ago by git

  • Commit changed from a092f680c401bf37778f2de00d373e895bee7643 to f332985f87eb116ae8d0806242da7d4efa96f3e7

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

f332985returned ellipses to doctests

comment:24 Changed 6 months ago by chapoton

  • Branch changed from u/gh-NebulousOinkler/fix_string_formatting_in_valueerror_in_sql_db_py to public/ticket/27562
  • Commit changed from f332985f87eb116ae8d0806242da7d4efa96f3e7 to 8c633267e64dc4643b5efd7b95cba86a4d0e174e
  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

I have moved the ellipses to the correct place, and then squashed the commits into a single commit: the result is the new branch.

I am setting to positive review.


New commits:

8c63326Fix string formatting in ValueError in sql_db.py

comment:25 Changed 6 months ago by vbraun

  • Branch changed from public/ticket/27562 to 8c633267e64dc4643b5efd7b95cba86a4d0e174e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.