Opened 7 years ago

Closed 7 years ago

#17045 closed defect (fixed)

Poset directed_subset() doesn't check for 2. argument

Reported by: jmantysalo Owned by:
Priority: trivial Milestone: sage-6.4
Component: combinatorics Keywords:
Cc: Merged in:
Authors: Jori Mäntysalo Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: b2ad198 (Commits, GitHub, GitLab) Commit: b2ad1980a0fc795728b0781a7270f8e0b26f04d9
Dependencies: Stopgaps:

Status badges

Description

Poset({0:[1]}).directed_subset([1], 'cat-says-meow')

does not give error message. Only 'up' and 'down' should be accepted.

Change History (16)

comment:1 Changed 7 years ago by jmantysalo

  • Branch set to u/jmantysalo/poset_directed_subset___doesn_t_check_for_2__argument

comment:2 Changed 7 years ago by jmantysalo

  • Commit set to a9336b45a7212825411ad37ac82cc3ec34df7db6
  • Status changed from new to needs_review

New commits:

a9336b4Added check: direction must be 'up' or 'down'.

comment:3 Changed 7 years ago by jmantysalo

  • Authors set to Jori Mäntysalo

comment:4 follow-up: Changed 7 years ago by ncohen

Could you close #17046 and make all modifications in this ticket ?

Nathann

comment:5 in reply to: ↑ 4 Changed 7 years ago by jmantysalo

Replying to ncohen:

Could you close #17046 and make all modifications in this ticket ?

Yes. Or at least try, let's see if it works.

comment:6 Changed 7 years ago by git

  • Commit changed from a9336b45a7212825411ad37ac82cc3ec34df7db6 to 37af4841c07479f7b628f3560f745f81b6858eca

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

37af484Merge #17046, i.e. same check for directed_subset() to this one.

comment:7 follow-up: Changed 7 years ago by ncohen

Thank you for merging them ! Usually we split tickets when they are very big and/or hard to review, or when they do very unrelated things, but in this case it's all very simple and it is less work for the release manager (who checks ALL of Sage's tests for every ticket, i.e. a LOT of computations).

Let me finish a ticket I am writing and I will review that immediately after.

Nathann

comment:8 in reply to: ↑ 7 Changed 7 years ago by jmantysalo

Replying to ncohen:

Let me finish a ticket I am writing and I will review that immediately after.

No hurry. This one would have "extremely trivial" priority if we would have it.

In future I'll try to make slightly bigger chunks when correcting minor things like this one.

comment:9 follow-up: Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

Hello again !

Two comments:

1) I'm sorry to say that if you write raise ValueError, message the same guys who complained about your "x <> y" last time will come and tell you that it is not "PEP-<whatever>". Could you change it into raise ValueError("message") ?

2) I do not believe that you need to modify finite_poset at all. Indeed, one function calls the other, so if there is anything wrong with respect to direction an exception will be raised anyway. What do you think ?

Nathann

comment:10 Changed 7 years ago by git

  • Commit changed from 37af4841c07479f7b628f3560f745f81b6858eca to b2ad1980a0fc795728b0781a7270f8e0b26f04d9

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

b2ad198Changed "raise type, text" to "raise type(text)".

comment:11 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Replying to ncohen:

1) I'm sorry to say that if you write raise ValueError, message the same guys who complained about your "x <> y" last time will come and tell you that it is not "PEP-<whatever>". Could you change it into raise ValueError("message") ?

Done this. Good to obey PEP-rules, nothing to be sorry about.

2) I do not believe that you need to modify finite_poset at all. Indeed, one function calls the other, so if there is anything wrong with respect to direction an exception will be raised anyway. What do you think ?

I don't like the idea. directed_subsets() return a function. Hence error message would be seen only after using it.

comment:12 in reply to: ↑ 11 Changed 7 years ago by ncohen

I don't like the idea. directed_subsets() return a function. Hence error message would be seen only after using it.

My mistake, I had not noticed.

Nathann

comment:13 Changed 7 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Thennnnnnnn it's good to go !

Thank you very much for taking the time to clean this code, by the way.

Nathann

comment:14 Changed 7 years ago by jmantysalo

  • Reviewers Nathann Cohen deleted

Btw, if raise type, text is against some PEP, then also http://www.sagemath.org/doc/thematic_tutorials/tutorial-programming-python.html#control-structures should be changed.

Also at least on graphs/graph_plot.py function layout_tree should be changed, see parameter orientation. I left that for now.

comment:15 Changed 7 years ago by ncohen

  • Reviewers set to Nathann Cohen

Well, change it if you like but as far as I am concerned this can all be changed automatically if we want to. Though apparently people also fear big change sets like that.

I just don't care much either way :-P

Nathann

comment:16 Changed 7 years ago by vbraun

  • Branch changed from u/jmantysalo/poset_directed_subset___doesn_t_check_for_2__argument to b2ad1980a0fc795728b0781a7270f8e0b26f04d9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.