Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(6)

Issue 431091003: tryton-env: Avoid ending_clause is_leaf evaluation to cause an infinite loop.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 3 days ago by hodeinavarro
Modified:
2 weeks, 1 day ago
Reviewers:
ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 1

Patch Set 2 : Use different approach and add test case #

Patch Set 3 : Add test case for tryton #

Total comments: 14

Patch Set 4 : I applied the remarks, simplified the tests and I think added the return of the simplify method to … #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -4 lines) Patch
M sao/src/common.js View 1 2 3 2 chunks +2 lines, -2 lines 9 comments Download
M sao/tests/sao.js View 1 2 3 1 chunk +18 lines, -0 lines 2 comments Download
M tryton/tryton/tests/test_common_domain_parser.py View 1 2 3 2 chunks +18 lines, -2 lines 2 comments Download

Messages

Total messages: 20
hodeinavarro
2 weeks, 3 days ago (2022-05-09 13:46:43 UTC) #1
ced
Could you provide a test case (also for tryton). Also avoid to use "Fix ..." ...
2 weeks, 3 days ago (2022-05-09 13:51:06 UTC) #2
reviewbot
checks OK URL: https://codereview.tryton.org/431091003
2 weeks, 3 days ago (2022-05-09 13:58:11 UTC) #3
hodeinavarro
On 2022/05/09 13:51:06, ced wrote: > Could you provide a test case (also for tryton). ...
2 weeks, 3 days ago (2022-05-09 14:04:39 UTC) #4
hodeinavarro
https://codereview.tryton.org/431091003/diff/429091003/src/common.js File src/common.js (right): https://codereview.tryton.org/431091003/diff/429091003/src/common.js#newcode1453 src/common.js:1453: if (this.is_leaf(last_element)) { Testing (last_element instanceof Array) inside this ...
2 weeks, 3 days ago (2022-05-09 14:09:33 UTC) #5
ced
On 2022/05/09 14:04:39, hodeinavarro wrote: > On 2022/05/09 13:51:06, ced wrote: > > Could you ...
2 weeks, 3 days ago (2022-05-09 14:28:47 UTC) #6
hodeinavarro
2 weeks, 3 days ago (2022-05-09 17:30:20 UTC) #7
hodeinavarro
On 2022/05/09 13:51:06, ced wrote: > Could you provide a test case (also for tryton). ...
2 weeks, 3 days ago (2022-05-09 17:44:36 UTC) #8
reviewbot
checks OK URL: https://codereview.tryton.org/431091003
2 weeks, 3 days ago (2022-05-09 17:48:28 UTC) #9
hodeinavarro
2 weeks, 3 days ago (2022-05-10 07:28:05 UTC) #10
reviewbot
checks OK URL: https://codereview.tryton.org/431091003
2 weeks, 3 days ago (2022-05-10 07:51:13 UTC) #11
ced
https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js File sao/tests/sao.js (right): https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js#newcode3104 sao/tests/sao.js:3104: }, I do not think any definition is needed. ...
2 weeks, 2 days ago (2022-05-11 09:33:00 UTC) #12
hodeinavarro
https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js File sao/tests/sao.js (right): https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js#newcode3104 sao/tests/sao.js:3104: }, On 2022/05/11 09:33:00, ced wrote: > I do ...
2 weeks, 1 day ago (2022-05-11 17:23:25 UTC) #13
hodeinavarro
I applied the remarks, simplified the tests and I think added the return of the ...
2 weeks, 1 day ago (2022-05-11 19:01:07 UTC) #14
reviewbot
checks OK URL: https://codereview.tryton.org/431091003
2 weeks, 1 day ago (2022-05-11 19:27:28 UTC) #15
ced
https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js File sao/tests/sao.js (right): https://codereview.tryton.org/431091003/diff/423121003/sao/tests/sao.js#newcode3114 sao/tests/sao.js:3114: '%name%', On 2022/05/11 17:23:25, hodeinavarro wrote: > On 2022/05/11 ...
2 weeks, 1 day ago (2022-05-11 21:13:22 UTC) #16
ced
https://codereview.tryton.org/431091003/diff/423141003/tryton/tryton/tests/test_common_domain_parser.py File tryton/tryton/tests/test_common_domain_parser.py (right): https://codereview.tryton.org/431091003/diff/423141003/tryton/tryton/tests/test_common_domain_parser.py#newcode1178 tryton/tryton/tests/test_common_domain_parser.py:1178: ('%C%', 2)) Please use subTest
2 weeks, 1 day ago (2022-05-11 21:14:00 UTC) #17
ced
https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js File sao/src/common.js (right): https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js#newcode1453 sao/src/common.js:1453: if ((last_element instanceof Array) && !this.is_leaf(last_element)) { 80 cols ...
2 weeks, 1 day ago (2022-05-11 21:31:34 UTC) #18
hodeinavarro
https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js File sao/src/common.js (right): https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js#newcode1453 sao/src/common.js:1453: if ((last_element instanceof Array) && !this.is_leaf(last_element)) { On 2022/05/11 ...
2 weeks, 1 day ago (2022-05-11 21:40:59 UTC) #19
ced
2 weeks, 1 day ago (2022-05-11 22:07:26 UTC) #20
https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js
File sao/src/common.js (right):

https://codereview.tryton.org/431091003/diff/423141003/sao/src/common.js#newc...
sao/src/common.js:2211: return this.simplify([value[1]]);
On 2022/05/11 21:40:59, hodeinavarro wrote:
> On 2022/05/11 21:31:33, ced wrote:
> > I do not see why it should be changed.
> 
> I've been checking multiple scenarios and the simplify function is returning
> consistently a list of a list or a list of multiple lists, except in this very
> scenario that returns the domain directly.
> 
> Following that "rule", this change is just for bringing consistency.

Please fix only bugs.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d9ca037-tainted