Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#26779 closed enhancement (fixed)

py3: fix graph_input.py and hypergraph_generators.py

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.5
Component: graph theory Keywords: py3, graph
Cc: tscrim, chapoton, embray Merged in:
Authors: David Coudert Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 016fa31 (Commits) Commit:
Dependencies: Stopgaps:

Description

doctest errors in hypergraph_generators.py are due to bytes vs str. We add a test in graph_input.py to fix the issue.

Change History (14)

comment:1 Changed 8 months ago by dcoudert

  • Branch set to public/26779_graph_input
  • Cc tscrim chapoton added
  • Commit set to c58ed7920a65bfde0e651f5393ca165db546bccf
  • Status changed from new to needs_review

I don't know if it's the best way to do it.

New commits:

​c58ed79trac #26779: fix hypergraph generators and graph input

comment:2 Changed 8 months ago by chapoton

  • Cc embray added

probably, it would be better to use "bytes_to_str" 

from sage.cpython.string import bytes_to_str

cc-ing Erik, who knows better about the unicode problem

comment:3 Changed 8 months ago by dcoudert

If it's safer I can use it. I was not aware of that method. Let me know if I should do the change.

comment:4 Changed 8 months ago by chapoton

yes, please use "bytes_to_str".

comment:5 Changed 8 months ago by git

  • Commit changed from c58ed7920a65bfde0e651f5393ca165db546bccf to 99e5b330baf3b8a5c7b16107ced5381cba83a2ea

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

​5f3b84dtrac #26779: Merged with 8.5.beta6
​99e5b33trac #26779: use bytes_to_str

comment:6 Changed 8 months ago by dcoudert

Thank you. I agree is cleaner this way.

Version 0, edited 8 months ago by dcoudert (next)

comment:7 Changed 7 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be

comment:8 Changed 7 months ago by embray

  • Status changed from positive_review to needs_work

Please try to avoid using inline imports if it isn't strictly necessary; see recent discussion about this at ​https://trac.sagemath.org/ticket/26758#comment:11

comment:9 Changed 7 months ago by git

  • Commit changed from 99e5b330baf3b8a5c7b16107ced5381cba83a2ea to 016fa31eb0083267eaccf15c5a508da5aaabbbb1

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

​016fa31trac #26779: move import to module level

comment:10 Changed 7 months ago by dcoudert

  • Status changed from needs_work to needs_review

I have not touched to existing imports. But it could also be done here if needed.

comment:11 Changed 7 months ago by chapoton

  • Status changed from needs_review to positive_review

ok

comment:12 Changed 7 months ago by vbraun

  • Branch changed from public/26779_graph_input to 016fa31eb0083267eaccf15c5a508da5aaabbbb1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 7 months ago by embray

  • Commit 016fa31eb0083267eaccf15c5a508da5aaabbbb1 deleted

It would also be great to squash trivial changes like those together, although at least in this case both versions are at least valid (it's more of a problem when one commit contains trivial errors).

comment:14 Changed 7 months ago by embray

Didn't mean to delete that. I think that's a bug in the trac plugin...

Note: See TracTickets for help on using tickets.