RE: GHC | Some refactoring in tcInferApps (!116)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RE: GHC | Some refactoring in tcInferApps (!116)

GHC - devs mailing list
GitLab

Devs

 

I’m at a loss for how to review GitLab changes.  Richard sent me the message below.  So I follow the link to “View on GitLab”, or I manually edit the URL to plain

https://gitlab.haskell.org/ghc/ghc/merge_requests/116

 

Either way, I can’t see any comments whatsoever!  It says 5/5 discussions resolved, but I can’t actually see them.  There is not “toggle discussion” button anywhere.

 

What should I do?  This is quite a big problem.

 

I suppose we could issue guidance NEVER to resolve a discussion, but that seems like the wrong conclusion.

 

Simon

 

From: Richard Eisenberg <[hidden email]>
Sent: 14 January 2019 03:03
To: Simon Peyton Jones <[hidden email]>
Subject: Re: GHC | Some refactoring in tcInferApps (!116)

 

Richard Eisenberg commented on a discussion on compiler/typecheck/TcHsType.hs:

1022

+                                 (vcat [ ppr ki_binder

1023

+                                       , ppr ki_arg

1024

+                                       , ppr (tyBinderType ki_binder)

1025

+                                       , ppr subst

1026

+                                       , ppr (isInvisibleBinder ki_binder) ])

1027

+                       ; ty_app_err ki_arg $ nakedSubstTy subst $

1028

+                                             mkPiTys all_ki_binders inner_ki }

1029

+

1030

+        -- no binder; try applying the substitution, or fail if that's not possible

1031

+      | otherwise

1032

+      = try_again_after_substing_or n subst fun inner_ki all_args $ \ substed_inner_ki ->

1033

+        ty_app_err ki_arg substed_inner_ki

1034

+

1035

+    go n subst fun all_ki_binders inner_ki all_args@(HsValArg arg : args)

1036

+      | ki_binder : ki_binders <- all_ki_binders

1037

+      = if isInvisibleBinder ki_binder

See if you like how I've done this now.


View it on GitLab.
You're receiving this email because of your account on gitlab.haskell.org. If you'd like to receive fewer emails, you can unsubscribe from this thread or adjust your notification settings.


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Mikolaj Konarski-2
> Either way, I can’t see any comments whatsoever!  It says 5/5 discussions resolved, but I can’t actually see them.  There is not “toggle discussion” button anywhere.

Hello Simon,

Just below "5/5 discussions resolved" I can see 5 discussions
with “toggle discussion” link to the right of each one.
Is that what you can't see, or do mean a single button
that toggles all resolved discussions (I can't see any)?
Or do you mean a button that un-resolves them permanently
(neither can I see one)?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: GHC | Some refactoring in tcInferApps (!116)

GHC - devs mailing list
Looking here
https://gitlab.haskell.org/ghc/ghc/merge_requests/116//diffs
I see literally NO discussions with a "toggle discussion" button.  Indeed if I search for "toggle" I get no hits.

When I initially clicked on "changes", the diff for TcHsType was initially collapsed; I had to click on "Click to expand it".

I can't account for why the behaviour is different for you.

Simon

|  -----Original Message-----
|  From: Mikolaj Konarski <[hidden email]>
|  Sent: 14 January 2019 09:46
|  To: Simon Peyton Jones <[hidden email]>
|  Cc: ghc-devs <[hidden email]>
|  Subject: Re: GHC | Some refactoring in tcInferApps (!116)
|  
|  > Either way, I can’t see any comments whatsoever!  It says 5/5 discussions
|  resolved, but I can’t actually see them.  There is not “toggle discussion”
|  button anywhere.
|  
|  Hello Simon,
|  
|  Just below "5/5 discussions resolved" I can see 5 discussions with “toggle
|  discussion” link to the right of each one.
|  Is that what you can't see, or do mean a single button that toggles all
|  resolved discussions (I can't see any)?
|  Or do you mean a button that un-resolves them permanently (neither can I
|  see one)?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Mikolaj Konarski-2
> Looking here
> https://gitlab.haskell.org/ghc/ghc/merge_requests/116//diffs
> I see literally NO discussions with a "toggle discussion" button.  Indeed if I search for "toggle" I get no hits.

Oh, I see, you are right with respect to the link you cited now.
None of the buttons help, just as you say. I have
to navigate back from the "Changes" to the "Discussion" tab,
which gets me to the link you gave initially
(https://gitlab.haskell.org/ghc/ghc/merge_requests/116)
where the discussions are available, but I can't see them
in the context of the whole diff and none of the buttons
there let me extend the view of the diff. That's quite suboptimal.

Could you confirm that at

https://gitlab.haskell.org/ghc/ghc/merge_requests/116

(without the 'diff' at the end) you can see the collapsed discussion items?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: GHC | Some refactoring in tcInferApps (!116)

GHC - devs mailing list
|  (without the 'diff' at the end) you can see the collapsed discussion items?

Yes, in the discussion tab I can.  But of course that is out of context; you just get a little (non-expandable) snippet.

Simon

|  -----Original Message-----
|  From: Mikolaj Konarski <[hidden email]>
|  Sent: 14 January 2019 10:30
|  To: Simon Peyton Jones <[hidden email]>
|  Cc: ghc-devs <[hidden email]>
|  Subject: Re: GHC | Some refactoring in tcInferApps (!116)
|  
|  > Looking here
|  > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
|  > ab.haskell.org%2Fghc%2Fghc%2Fmerge_requests%2F116%2F%2Fdiffs&amp;data=
|  > 02%7C01%7Csimonpj%40microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7
|  > C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636830586093208883&amp;sda
|  > ta=qAcz0zoXCCQHFeJ39ABKAVwtN5a2pIwyRSbfJcLSurk%3D&amp;reserved=0
|  > I see literally NO discussions with a "toggle discussion" button.  Indeed
|  if I search for "toggle" I get no hits.
|  
|  Oh, I see, you are right with respect to the link you cited now.
|  None of the buttons help, just as you say. I have to navigate back from the
|  "Changes" to the "Discussion" tab, which gets me to the link you gave
|  initially
|  (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.h
|  askell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40
|  microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7c
|  d011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm
|  4ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0)
|  where the discussions are available, but I can't see them in the context of
|  the whole diff and none of the buttons there let me extend the view of the
|  diff. That's quite suboptimal.
|  
|  Could you confirm that at
|  
|  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.ha
|  skell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40m
|  icrosoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7cd
|  011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm4
|  ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0
|  
|  (without the 'diff' at the end) you can see the collapsed discussion items?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Mikolaj Konarski-2
> Yes, in the discussion tab I can.  But of course that is out of context; you just get a little (non-expandable) snippet.

I lack the gitlab experience to push this any further, but I guess
an "unresolve discussion" button/checkbox would at least be
a stop-gap measure. I couldn't see such a button on the page,
probably because I don't have the required permissions for this MR.
However, the video in the ticket below (and lots of noise
elsewhere on the net) suggests such a button might exists.
Ben, or anybody, do you know anything more about that?

https://gitlab.com/gitlab-org/gitlab-ce/issues/53930
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Mikolaj Konarski-2
In reply to this post by GHC - devs mailing list
On Mon, Jan 14, 2019 at 10:28 AM Simon Peyton Jones via ghc-devs <[hidden email]> wrote:
>
> I suppose we could issue guidance NEVER to resolve a discussion, but that seems like the wrong conclusion.

It seems gitlab's "resolve discussion" action is supposed to mean
"I think nobody needs to look at these comments (in detail) any more".

So if somebody addressed your comment, he should probably
ping you with "this comment addressed", then you look at the comment,
at the old diff, at the new diff (no idea how easy it is to switch between
the old and new diffs) and then *you* resolve the discussion, if satisfied.
And if somebody resolves discussion prematurely, you unresolve it
with the button or checkbox, meaning "actually, I'd like to have
one more close look before the discussion is hidden forever".
You can even add an unresolve comment explaining why you think
the additional look is needed after all.

All this is quite troublesome if there is a lot of comments
and they need to be pinged about, resolved and unresolved
in bulk (unless I'm missing some bulk button).


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Richard Eisenberg-4
I confirm that I see "Unresolve discussion" buttons in the "Discussion" page, after "Toggle Discussion" is clicked.

Perhaps we have this working convention: the dev that starts the discussion is expected to resolve it. I doubt GL can enforce that convention, but we can just agree to it all. So, in the case at hand, I wrote the patch, and Simon started several discussions. As I address them, instead of resolving the discussions (as I did), I would just say "Done" (or similar). Then, Simon's re-review would spot my comments and then click resolve, when the matter has met his satisfaction.

Ben: I imagine this will not be the only such convention we arrive at in this vein. Is it possible (and reasonably easy) to set up some service whereby new contributors get a "welcome" email upon their first submission of an MR? (But only upon the *first* submission!) This would include hearty thanks for contributing, etc., but also a few GL pointers and our working conventions. To be honest, even forgetting about working conventions, an email of thanks at that stage would probably set the stage for a nice working environment for contributors. (Right now, they probably submit, get some silence, then see a slew of comments telling them what they've done wrong, and then get a "Thanks so much!" after all those negative comments. This may be somewhat standard in open-source -- and isn't so bad that it *needs* change -- but that doesn't mean we can't do better.)

Richard

On Jan 14, 2019, at 7:30 AM, Mikolaj Konarski <[hidden email]> wrote:

On Mon, Jan 14, 2019 at 10:28 AM Simon Peyton Jones via ghc-devs <[hidden email]> wrote:
>
> I suppose we could issue guidance NEVER to resolve a discussion, but that seems like the wrong conclusion.

It seems gitlab's "resolve discussion" action is supposed to mean
"I think nobody needs to look at these comments (in detail) any more".

So if somebody addressed your comment, he should probably
ping you with "this comment addressed", then you look at the comment,
at the old diff, at the new diff (no idea how easy it is to switch between
the old and new diffs) and then *you* resolve the discussion, if satisfied.
And if somebody resolves discussion prematurely, you unresolve it
with the button or checkbox, meaning "actually, I'd like to have
one more close look before the discussion is hidden forever".
You can even add an unresolve comment explaining why you think
the additional look is needed after all.

All this is quite troublesome if there is a lot of comments
and they need to be pinged about, resolved and unresolved
in bulk (unless I'm missing some bulk button).

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Sylvain Henry-2
In reply to this post by GHC - devs mailing list

Hi Simon,

In the diff view, you can click on the avatars on the left to expand discussions:

Or you can click on the "toggle comments for this file" button:



If you are willing to use GreaseMonkey (Firefox extension) or TamperMonkey (Chrome/Chromium extension), you can use the attached script to add new buttons to GitLab UI:


Tested on Firefox and Chromium on Linux.

Sylvain




On 14/01/2019 11:58, Simon Peyton Jones via ghc-devs wrote:
|  (without the 'diff' at the end) you can see the collapsed discussion items?

Yes, in the discussion tab I can.  But of course that is out of context; you just get a little (non-expandable) snippet.

Simon

|  -----Original Message-----
|  From: Mikolaj Konarski [hidden email]
|  Sent: 14 January 2019 10:30
|  To: Simon Peyton Jones [hidden email]
|  Cc: ghc-devs [hidden email]
|  Subject: Re: GHC | Some refactoring in tcInferApps (!116)
|  
|  > Looking here
|  > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
|  > ab.haskell.org%2Fghc%2Fghc%2Fmerge_requests%2F116%2F%2Fdiffs&amp;data=
|  > 02%7C01%7Csimonpj%40microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7
|  > C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636830586093208883&amp;sda
|  > ta=qAcz0zoXCCQHFeJ39ABKAVwtN5a2pIwyRSbfJcLSurk%3D&amp;reserved=0
|  > I see literally NO discussions with a "toggle discussion" button.  Indeed
|  if I search for "toggle" I get no hits.
|  
|  Oh, I see, you are right with respect to the link you cited now.
|  None of the buttons help, just as you say. I have to navigate back from the
|  "Changes" to the "Discussion" tab, which gets me to the link you gave
|  initially
|  (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.h
|  askell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40
|  microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7c
|  d011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm
|  4ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0)
|  where the discussions are available, but I can't see them in the context of
|  the whole diff and none of the buttons there let me extend the view of the
|  diff. That's quite suboptimal.
|  
|  Could you confirm that at
|  
|  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.ha
|  skell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40m
|  icrosoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7cd
|  011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm4
|  ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0
|  
|  (without the 'diff' at the end) you can see the collapsed discussion items?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

gitlab_ghc.user.js (1016 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: GHC | Some refactoring in tcInferApps (!116)

GHC - devs mailing list
In reply to this post by GHC - devs mailing list

Thanks.  But none of the pictures arrived, so I can’t interpret what you say.

 

I think I did see Richards face here and there (and mine), if that’s want avatars means. Hovering showed a snippet of a comment; but clicking did nothing.

 

I’m also terribly reluctant to scroll through thousands of lines of diff looking for avarars. 

 

Simon

 

From: ghc-devs <[hidden email]> On Behalf Of Sylvain Henry
Sent: 14 January 2019 16:21
To: [hidden email]
Subject: Re: GHC | Some refactoring in tcInferApps (!116)

 

Hi Simon,

In the diff view, you can click on the avatars on the left to expand discussions:

Or you can click on the "toggle comments for this file" button:

 

 

If you are willing to use GreaseMonkey (Firefox extension) or TamperMonkey (Chrome/Chromium extension), you can use the attached script to add new buttons to GitLab UI:

 

Tested on Firefox and Chromium on Linux.

Sylvain

 

 

 

On 14/01/2019 11:58, Simon Peyton Jones via ghc-devs wrote:

|  (without the 'diff' at the end) you can see the collapsed discussion items?
 
Yes, in the discussion tab I can.  But of course that is out of context; you just get a little (non-expandable) snippet.
 
Simon
 
|  -----Original Message-----
|  From: Mikolaj Konarski [hidden email]
|  Sent: 14 January 2019 10:30
|  To: Simon Peyton Jones [hidden email]
|  Cc: ghc-devs [hidden email]
|  Subject: Re: GHC | Some refactoring in tcInferApps (!116)
|  > Looking here
|  > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl
|  > ab.haskell.org%2Fghc%2Fghc%2Fmerge_requests%2F116%2F%2Fdiffs&amp;data=
|  > 02%7C01%7Csimonpj%40microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7
|  > C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636830586093208883&amp;sda
|  > ta=qAcz0zoXCCQHFeJ39ABKAVwtN5a2pIwyRSbfJcLSurk%3D&amp;reserved=0
|  > I see literally NO discussions with a "toggle discussion" button.  Indeed
|  if I search for "toggle" I get no hits.
|  Oh, I see, you are right with respect to the link you cited now.
|  None of the buttons help, just as you say. I have to navigate back from the
|  "Changes" to the "Discussion" tab, which gets me to the link you gave
|  initially
|  (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.h
|  askell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40
|  microsoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7c
|  d011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm
|  4ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0)
|  where the discussions are available, but I can't see them in the context of
|  the whole diff and none of the buttons there let me extend the view of the
|  diff. That's quite suboptimal.
|  Could you confirm that at
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.ha
|  skell.org%2Fghc%2Fghc%2Fmerge_requests%2F116&amp;data=02%7C01%7Csimonpj%40m
|  icrosoft.com%7Cb565eeb2f6fe445fc64f08d67a0b40fe%7C72f988bf86f141af91ab2d7cd
|  011db47%7C1%7C0%7C636830586093208883&amp;sdata=1pFmcwPyptWE8XOs%2FdnykEqsm4
|  ag7%2BqBi3c4qpAE7sE%3D&amp;reserved=0
|  (without the 'diff' at the end) you can see the collapsed discussion items?
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Mikolaj Konarski-2
Richard, what you propose totally makes sense to me.

Simon, Sylvain, indeed, when I click "toggle comments for this file"
in the first patch, I can see not only an avatar, but the whole comment.
However, this is only one comment, in both diffs, out of many,
which adds to the confusion. May it be caused by new changes
invalidating old comments? That's an orthogonal issue, which I remember
being quite annoying on github.

Also, with "toggle comments for this file" unmarked (it's really hard to tell
what the state of that button is), I can see the tiny avatar and, for me,
just clicking on it reveals the full comment.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: GHC | Some refactoring in tcInferApps (!116)

Sylvain Henry-2
In reply to this post by GHC - devs mailing list

> Thanks.  But none of the pictures arrived, so I can’t interpret what you say.

They probably have been filtered by the ML...

They can seen with the script here: https://gist.github.com/hsyl20/912b0a5fec9e7c621d8ac82e46b88d93




_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs