search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Remove superfluous parenthesis around assert #888

Closed scravy closed 5 years ago

scravy commented 5 years ago

Follow-up to https://github.com/dtr-org/unit-e/pull/859#discussion_r270504646

Signed-off-by: Julian Fleischer julian@thirdhash.com

kostyantyn commented 5 years ago

Searched for assert( and found a couple of such cases, maybe we can drop them all in this PR?

scravy commented 5 years ago

@kostyantyn Did it, using IntelliJ and Regex – see 2d1d7bc93a17578636f98eb0fdd91c8cf0305e4a

Please review thoroughly. I did review the changeset myself but there are curious cases like

assert (a or b) and (c or d)

which were turned into

assert a or b) and (c or d

I think I caught all of them, but please double-check.

frolosofsky commented 5 years ago

Why do we do that for bitcoin code? This changes git history and could complicate some researches. Also I find assert(x) is much more consistent with our others use cases like assert_equal(a, b), dunno why should we distinguish these cases.

scravy commented 5 years ago

Why do we do that for bitcoin code? This changes git history and could complicate some researches. Also I find assert(x) is much more consistent with our others use cases like assert_equal(a, b), dunno why should we distinguish these cases.

@kostyantyn You suggested.

@cmihai Now that you gathered the experience - do you think this is a problem with merging upstream ongoingly

kostyantyn commented 5 years ago

@frolosofsky @scravy Bitcoin ran the script-diff to do the same. If we care about the history we can cherry-pick that commit. https://github.com/bitcoin/bitcoin/commit/fa0e65b77264476c61832542ab1a9dbedcc738ea#diff-695d736c7d2a38a42ae900546aed52f8

About the parentheses itself, it's dangerous to keep as it is. Here is an example:

➜  unit-e git:(commits-and-snapshot) python3
Python 3.7.1 (default, Nov  6 2018, 18:46:03)
...
>>> assert(False==True, "Something wrong")
<stdin>:1: SyntaxWarning: assertion is always true, perhaps remove parentheses?
>>>

The issue is if you want to add the description to the assert, you might be fooled and add it inside the () which will create the pair and it always resolves to True. It's because assert is the keyword, assert_equal is the function so they have different syntax.

cmihai commented 5 years ago

@scravy When both Bitcoin and we change a line, it results in a merge conflict. So I'm all for cherry-picking the commit indicated by @kostyantyn to avoid more headache in the future.

Edit: however, once we resolve the conflict, it won't be a problem next time we merge.

scravy commented 5 years ago

@scravy When both Bitcoin and we change a line, it results in a merge conflict. So I'm all for cherry-picking the commit indicated by @kostyantyn to avoid more headache in the future.

Edit: however, once we resolve the conflict, it won't be a problem next time we merge.

But it's nevertheless straight forward, right? Also we should probably not just cherry-pick their commit but re-apply their scripted diff.

Cherry picking commits which ground on changes and with the clonemachine and everything is not always the solution IMHO.

cmihai commented 5 years ago

Yes, resolving any conflicts would be straightforward, and in the end, it makes no difference who removed the parentheses, since it's a trivial change. We can definitely apply the scripted diff instead of cherry-picking, then.

scravy commented 5 years ago

Yes, resolving any conflicts would be straightforward, and in the end, it makes no difference who removed the parentheses, since it's a trivial change. We can definitely apply the scripted diff instead of cherry-picking, then.

Applying the scripted diff would result in the same commit as this one. A scripted diff mostly has the virtue that it is easier reviewable (in case of this particular scripted diff I wonder about this, you need to delve into regex land quite deeply). Now this has been extensively reviewed and passed the tests already.

Would you like me to prepare a scripted diff nevertheless?

kostyantyn commented 5 years ago

From my hand, It's better to keep as it is because the author of the commit will be the same and I already went through every line, so won't do it again :) Would prefer to check only commits on top of this one :)

cornelius commented 5 years ago

If the changes done on both sides give identical results we shouldn't get conflicts when merging unless they are close to other potentially conflicting changes, but we will get both commits in the history after the upstream merge. So I don't see harm in applying this patch. ConceptACK.

scravy commented 5 years ago

Seems to me it's okay to merge this as is.