URL: https://github.com/SSSD/sssd/pull/250 Author: lslebodn Title: #250: cache_req: Use ternary operator instead of bool+switch Action: opened
PR body: """ IMHO it looks little bit better. I know there is explanation but would like to see other opinion. @fidencio what do you think? """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/250/head:pr250 git checkout pr250
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ @lslebodn, I've chosen the switch just to make the code cleaner, as I do believe the cases are easier to read than the ternary operator.
However, if you think it's easier to read using the ternary operator, I won't oppose to it.
"""
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297653907
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """ I think there is a big difference between 11 lines and 3 lines. `switch + boolean` does not simplify it.
It might improve readability a bit in case of complicated boolean expression. But it does not simplify it in this case (`true`/`false`/value of variable) """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297667663
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """ I think there is a big difference between 11 lines and 3 lines. `switch + boolean` does not simplify it.
It might improve readability a bit in case of complicated boolean expression. But it does not simplify it in this case (`true`/`false`/value of variable) """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297667663
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
pbrezina commented: """ I, personally, would have used if/else. But between these two, I found switch more readable then nested ternary operator. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297668738
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
mzidek-rh commented: """ What about: ``` if (head->fqnames) { return true; } else if (enforce_non_fqnames) { return false; } else { return domain->fqnames; } ```
I also do not think the switch is very good choice in case of boolean true/false values. I am Ok with the Lukáš's version, but I like the "if - else if - else" version even better. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297673939
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ Personally I think any of the suggestions make the code less readable than the switch (which as all the cases explicitly matching with the comments above).
For me, it's a nack for the patch and for those suggestions. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297676645
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ Personally I think any of the suggestions make the code less readable than the switch (which as all the cases explicitly matching with the comments above).
For me, it's a nack for the patch and for those suggestions. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297676645
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ Personally I think any of the suggestions make the code more readable than the switch (which as all the cases explicitly matching with the comments above).
For me, it's a nack for the patch and for those suggestions. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297676645
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """
I Personally I think any of the suggestions make the code less readable than the switch (which as all the cases explicitly matching with the comments above).
I'm sorry but switch is the worst option from proposed.
It looks like 2 guys like version with `if`. I will prepare version with `if` even though that ternary is more readable :-). Democracy works. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297701355
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """
I Personally I think any of the suggestions make the code less readable than the switch (which as all the cases explicitly matching with the comments above).
I'm sorry but switch is the worst option from proposed.
It looks like 2 guys like version with `if`. I will prepare version with `if` even though that ternary is more readable :-). Democracy works. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297701355
URL: https://github.com/SSSD/sssd/pull/250 Author: lslebodn Title: #250: cache_req: Use ternary operator instead of bool+switch Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/250/head:pr250 git checkout pr250
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ I still don't think it makes anything cleaner, just reduce the amount of lines.
It's a nack from me, but if someone else feels like acking it ... I won't oppose either. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297717452
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """
I still don't think it makes anything cleaner, just reduce the amount of lines.
The main purpose is not reduction of lines. But make it more readable. After the discussion, it was decided that switch+bool is the less readable form proposed options. (just you like it :-) """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297798113
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ No, it was decided that just other people like the if more, not that it is more readable.
Remember that forcing your opinion on a NACK will be taken into consideration when you'll be NACKing something ... (which is fine by me, but may not be for you). """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297916561
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """
No, it was decided that just other people like the if more, not that it is more readable.
If they they like it more then it is more readable.
Remember that forcing your opinion on a NACK will be taken into consideration when you'll be NACKing something ... (which is fine by me, but may not be for you).
If I saw switch + bool earlier you would request change as part of review. But I do not have a resources to review all patches these days due to other tasks. And in this case, your NACK does not count because there are 3 other developers which prefer different version. (3 vs 1) Sorry democracy works. The same applies to any other developer(including me) NACK cannot beet 3+ developers :-) Because reasonable NACK with proper explanation would persuade them.
"""
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297930326
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
fidencio commented: """ Okay, I agree that democracy should win, in all cases :-)
I'm acking the patch and keeping this PR on track for future uses where the basic democratic rule is not used.
Please, run the CI on this very simple patch before pushing it. """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297933731
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
mzidek-rh commented: """ LGTM++ """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-297988213
URL: https://github.com/SSSD/sssd/pull/250 Author: lslebodn Title: #250: cache_req: Use ternary operator instead of bool+switch Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/250/head:pr250 git checkout pr250
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
lslebodn commented: """ http://sssd-ci.duckdns.org/logs/job/68/84/summary.html
master: * b70ec63cca034ce9c638f2dcefbd4358b654d2e1 """
See the full comment at https://github.com/SSSD/sssd/pull/250#issuecomment-298153558
URL: https://github.com/SSSD/sssd/pull/250 Title: #250: cache_req: Use ternary operator instead of bool+switch
Label: +Pushed
sssd-devel@lists.fedorahosted.org