Opened 5 years ago

Closed 5 years ago

#19349 closed enhancement (fixed)

Apply #18731 again

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.10
Component: build Keywords:
Cc: jhpalmieri Merged in:
Authors: Jeroen Demeyer Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 9258278 (Commits) Commit: 92582788c1f3a01479f4414b466275120aa843ec
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Some modifications made in #18731 disappeared in #18748. These should be applied again.

Change History (11)

comment:1 Changed 5 years ago by jdemeyer

  • Cc jhpalmieri added

comment:2 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/apply__18731_again

comment:3 Changed 5 years ago by jdemeyer

  • Commit set to 5a0c4b0d9610c2eee9f8d25f929ae3047cb65df8
  • Status changed from new to needs_review

New commits:

​5a0c4b0Better error handling in sage-download-file

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 5 years ago by vbraun

Magic string anti-pattern: ​https://en.wikipedia.org/wiki/Magic_string

comment:6 Changed 5 years ago by git

  • Commit changed from 5a0c4b0d9610c2eee9f8d25f929ae3047cb65df8 to 088fdefb1e91ae460f67fa31344f6cb834d0b117

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

​088fdefAvoid magic string

comment:7 Changed 5 years ago by vbraun

Its still a magic string. If somebody were to change the magic value to 

fastest_mirror = "http:// fastest mirror"

then the program doesn't work any more. Fragile and difficult to follow.

comment:8 Changed 5 years ago by git

  • Commit changed from 088fdefb1e91ae460f67fa31344f6cb834d0b117 to 92582788c1f3a01479f4414b466275120aa843ec

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

​9258278Avoid magic string

comment:9 Changed 5 years ago by jdemeyer

Better like this?

comment:10 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

The usual pattern is to throw exceptions that have the error message and catch them at the outermost level. Without relying on any local variable being in scope at the exception handler. But its an improvement, at least.

comment:11 Changed 5 years ago by vbraun

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