Patchwork D6123: Experimental features: Add condition to for float number (issue6099)

login
register
mail settings
Submitter phabricator
Date March 12, 2019, 6:09 p.m.
Message ID <differential-rev-PHID-DREV-n4ecb5gpvir3xbcstvyr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39231/
State New
Headers show

Comments

phabricator - March 12, 2019, 6:09 p.m.
akshjain.jain74 created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

AFFECTED FILES
  hgext/histedit.py
  mercurial/similar.py

CHANGE DETAILS




To: akshjain.jain74, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2019, 8:54 a.m.
pulkit added inline comments.

INLINE COMMENTS

> histedit.py:1127
>      else:
> -        index += 1
> +        index -= 1
>      changeaction(state, pos, KEY_LIST[index % len(KEY_LIST)])

Is this change required for this patch?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 13, 2019, 9:17 a.m.
akshjain.jain74 added a comment.


  Oh sorry ,my bad  I think that is by mistake I will correct it now
  Ithink while committing I added that file by mistake

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 13, 2019, 9:20 a.m.
av6 added a comment.


  I find it troubling that we now have contributors that don't follow #1 in https://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist. Potentially #4 too (as Pulkit commented).
  
  Seriously, the title is terrible at describing what this patch fixes and where. The commit message could briefly mention why/when this condition is even required.

INLINE COMMENTS

> similar.py:66-67
>      lengths = len(text) + len(orig)
> +    if lengths > 0:
>      return equal * 2.0 / lengths
>  

This is some... interesting syntax. Is this valid Python?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 13, 2019, 9:23 a.m.
av6 added inline comments.

INLINE COMMENTS

> similar.py:67
> +    if lengths > 0:
>      return equal * 2.0 / lengths
>  

This returns None in some cases, and code that uses `_score()` and `score()` tries to compare it to an integer. In Python3 `None > 1` raises TypeError.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 13, 2019, 9:47 a.m.
akshjain.jain74 added a comment.


  Sorry for the various mistake
   I will correct everything in an hour thanks

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 13, 2019, 6:02 p.m.
av6 added a comment.


  Okay, let's go over #1 in https://www.mercurial-scm.org/wiki/ContributingChanges#Submission_checklist once more. If you want to know what a good "topic" is, look at what other people do. How patches that get accepted generally look. How bug-fixing commits are worded.
  
  Secondly, about that capitalization. I'm sorry if this is some sort of silly autocorrect that's doing it for you, I really hope you're not writing commit messages on a phone.

INLINE COMMENTS

> similar.py:66-67
>      lengths = len(text) + len(orig)
> -    return equal * 2.0 / lengths
> +    if lengths > 0:
> +        return equal * 2.0 / lengths
>  

`None > 1` is still an issue, see my comment above.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 13, 2019, 6:14 p.m.
akshjain.jain74 added a comment.


  sorry for that , but can you tell me in what condition this function will return none value?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 13, 2019, 6:16 p.m.
akshjain.jain74 added a comment.


  actually  i am not getting what can be the proper topic for this :(

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 15, 2019, 1:44 p.m.
pulkit added a comment.


  Can you try to add a test for this?

INLINE COMMENTS

> similar.py:66
>      lengths = len(text) + len(orig)
> -    return equal * 2.0 / lengths
> +    if lengths != 0:
> +        return equal * 2.0 / lengths

we can do `if lengths:` here.

> similar.py:68
> +        return equal * 2.0 / lengths
> +    else:
> +        return 0

no need for this else. you can `return 0` without else.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 15, 2019, 1:57 p.m.
akshjain.jain74 added a comment.


  In https://phab.mercurial-scm.org/D6123#89410, @pulkit wrote:
  
  > Can you try to add a test for this?
  
  
  yes i can definitely try to add test for this

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel
phabricator - March 15, 2019, 1:58 p.m.
akshjain.jain74 added a comment.


  thanks for the review

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6123

To: akshjain.jain74, durin42, #hg-reviewers
Cc: av6, pulkit, mercurial-devel

Patch

diff --git a/mercurial/similar.py b/mercurial/similar.py
--- a/mercurial/similar.py
+++ b/mercurial/similar.py
@@ -63,6 +63,7 @@ 
             equal += len(line)
 
     lengths = len(text) + len(orig)
+    if lengths > 0:
     return equal * 2.0 / lengths
 
 def score(fctx1, fctx2):
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1124,7 +1124,7 @@ 
     if next:
         index += 1
     else:
-        index += 1
+        index -= 1
     changeaction(state, pos, KEY_LIST[index % len(KEY_LIST)])
 
 def changeview(state, delta, unit):