***important*** cannot remove a class anymore...
Locked
123
123
 
|
As you wish. Thanks for your time.
Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion to learn something. This is what sending the diff in text simply give us. Stef Le 5/9/15 14:09, Yuriy Tymchuk a écrit : > I found the issue. Fix almost ready. Should I reopen the case, or create a new one? > > Uko > > > >> On 05 Sep 2015, at 13:59, Stephan Eggermont <[hidden email]> wrote: >> >> On 05-09-15 08:51, stepharo wrote: >>> Hi >>> >>> I do not know if this is linked to recent changes but we cannot remove >>> classes or move them to another package >> I can move them to another package using drag-and-drop (issue 16488) >> in 50305 >> >> >> Stephan >> >> > > |
|
|
In reply to this post by Sven Van Caekenberghe-2
>> On 05 Sep 2015, at 13:29, Yuriy Tymchuk <[hidden email]> wrote: >> >> You can see changes for that patch here: >> https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e29cb055d108731c8 > YES, we have that for years now. and how many people read one change? > True, it is one click away, but the view is quite nice (not something that can be reproduced in an email). No it is not because I delete the mail with these headers and I cannot find a list on the web to find the diff. > >>> On 05 Sep 2015, at 13:23, stepharo <[hidden email]> wrote: >>> >>> >>>>> On 05 Sep 2015, at 11:40, stepharo <[hidden email]> wrote: >>>>> >>>>> How can I see the changes? >>>>> Our process is not good. Most of us do not get any chance understanding what is changing. >>>>> >>>> -> download the image before it was added >>>> -> merge the slice. >>>> >>>> Yes, our process is not good… but from a review perspective, this issue is the best we can do. >>>> *two* reviews, both from people actively contributing to exactly that part of the system. >>>> >>>> If we require more, we will be back at a process where due to Fear we do nothing. >>> I do not want to require more. I want to see all the changes that we are doing without having to download an image >>> and doing merge. >>> I read many squeak changes like that and IT WORKS. >>> >>> Stef >>>>>> Hi, >>>>>> >>>>>> This is a side effect of >>>>>> >>>>>> https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcements-for-a-single-action >>>>>> >>>>>> (which was reviewed by two people, so not obvious). >>>>>> >>>>>> What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop. >>>>>> >>>>>> Should be easy to fix for the people involved in case 16475. >>>>>> >>>>>> Marcus >>>>>> >>>>>> >>>>>> On Sat, Sep 5, 2015 at 8:51 AM, stepharo <[hidden email]> wrote: >>>>>> Hi >>>>>> >>>>>> I do not know if this is linked to recent changes but we cannot remove classes or move them to another package. >>>>>> >>>>>> Stef >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> -- >>>>>> Marcus Denker -- [hidden email] >>>>>> http://www.marcusdenker.de > > |
|
|
In reply to this post by stepharo
I can give you a short explanation for the sake of more people knowing how Nautilus works.
Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit. Uko > On 05 Sep 2015, at 15:46, stepharo <[hidden email]> wrote: > > As you wish. Thanks for your time. > > Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion > to learn something. This is what sending the diff in text simply give us. > > Stef > > > Le 5/9/15 14:09, Yuriy Tymchuk a écrit : >> I found the issue. Fix almost ready. Should I reopen the case, or create a new one? >> >> Uko >> >> >> >>> On 05 Sep 2015, at 13:59, Stephan Eggermont <[hidden email]> wrote: >>> >>> On 05-09-15 08:51, stepharo wrote: >>>> Hi >>>> >>>> I do not know if this is linked to recent changes but we cannot remove >>>> classes or move them to another package >>> I can move them to another package using drag-and-drop (issue 16488) >>> in 50305 >>> >>> >>> Stephan >>> >>> >> >> > > |
Re: ***important*** cannot remove a class anymore...
|
|
In reply to this post by Marcus Denker-4
2015-09-05 11:48 GMT+02:00 Marcus Denker <[hidden email]>:
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof. nicolai
|
|
|
In fact you couldn’t spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it I’ve spent probably 1.5 hours :)
Uko
|
Re: ***important*** cannot remove a class anymore...
|
|
Just because looking at a patch isn't as powerful doesn't make it useless. There are still benefits
* you are more aware of code changes * you can still spot conceptual problems * it's dead simple; the mail literally arrives at your doorsteps and all you need to do is look of course this isn't replacing proper review, but there is big still value in it. But even the "regular" process could be improved.
For example I have startup script, that for images that have name "issuenumber - verify" on startup automatically opens MCBrowser on the slice so I can merge with two clicks. (It's not automerged because I want to test behavior before and after loading the slice.) Since I'm mostly verifying my own fixes, I generally don't look at the changesets, but maybe you could autoopen a diff or something... Or even add a category to PharoLauncher "Pharo - To Review" so I wouldn't even need to type in the issue number. Peter |
|
|
In reply to this post by Nicolai Hess
Oh yes I know. I'm not saying that looking at the changes we would
have spotted the problems.
I'm just saying that I want to get the changes in textual format in my mail-box as in squeak because it helps me understanding and knowing that something changed. I'm digging in the Diffbuilder and friends to see but I have some other tasks to finish first.
|
|
|
In reply to this post by Uko2
Le 5/9/15 19:18, Yuriy Tymchuk a
écrit :
In fact you couldn’t spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it I’ve spent probably 1.5 hours :) sure this is not my point. my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. I want to see diffs right in front of my nose. Stef
|
|
|
In reply to this post by Peter Uhnak
Le 5/9/15 20:46, Peter Uhnák a écrit :
100% what I want to say. I will allocate time on a goodies that take a slice and produce a diff
|
|
|
In reply to this post by Uko2
Le 5/9/15 16:49, Yuriy Tymchuk a écrit : > I can give you a short explanation for the sake of more people knowing how Nautilus works. Yes please if you have some time. What I do not like is that we pass nil to indicate that there is no selection. I was thinking that an hierarchy of null objects could help but this is difficult to foresee if it will make sense. I also that all the logic update without reselecting is complex. I started to read the code and also move some condition about the model to the model class :). I would like to do just a simple pass reading all the code and not changing much to get now yet another better feel of nautilus. I will also remove the Nautilus help. I would like to rename the NautilusRefactoring because it is not about nautilus but about RB. > > Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit. thomas was working on a code review tool and skip is taking over so I hope that we will have it. > > Uko > >> On 05 Sep 2015, at 15:46, stepharo <[hidden email]> wrote: >> >> As you wish. Thanks for your time. >> >> Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion >> to learn something. This is what sending the diff in text simply give us. >> >> Stef >> >> >> Le 5/9/15 14:09, Yuriy Tymchuk a écrit : >>> I found the issue. Fix almost ready. Should I reopen the case, or create a new one? >>> >>> Uko >>> >>> >>> >>>> On 05 Sep 2015, at 13:59, Stephan Eggermont <[hidden email]> wrote: >>>> >>>> On 05-09-15 08:51, stepharo wrote: >>>>> Hi >>>>> >>>>> I do not know if this is linked to recent changes but we cannot remove >>>>> classes or move them to another package >>>> I can move them to another package using drag-and-drop (issue 16488) >>>> in 50305 >>>> >>>> >>>> Stephan >>>> >>>> >>> >> > > |
Re: ***important*** cannot remove a class anymore...
|
|
Le 05/09/2015 21:39, stepharo a écrit :
> > > Le 5/9/15 16:49, Yuriy Tymchuk a écrit : >> I can give you a short explanation for the sake of more people knowing >> how Nautilus works. > Yes please if you have some time. > What I do not like is that we pass nil to indicate that there is no > selection. I was thinking that an hierarchy of null objects could help > but this is difficult to > foresee if it will make sense. > I also that all the logic update without reselecting is complex. > I started to read the code and also move some condition about the model > to the model class :). > I would like to do just a simple pass reading all the code and not > changing much to get now yet another better feel of nautilus. > I will also remove the Nautilus help. > I would like to rename the NautilusRefactoring because it is not about > nautilus but about RB. > >> >> Regarding diffs and knowledge sharing, we should have a normal code >> review support with something like gerrit. > > thomas was working on a code review tool and skip is taking over so I > hope that we will have it. >> >> Uko >> > > -- Cyril |
signature.asc (836 bytes)
Re: ***important*** cannot remove a class anymore...
|
|
In reply to this post by stepharo
> On 05 Sep 2015, at 21:34, stepharo <[hidden email]> wrote: > > > > Le 5/9/15 19:18, Yuriy Tymchuk a écrit : >> In fact you couldn’t spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it I’ve spent probably 1.5 hours :) > > sure this is not my point. > my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. > I want to see diffs right in front of my nose. I know very little about git, but the first thing that I tried worked: git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8 I assume that we could put that textual information in the mail. In my terminal, it is coloured, but I still think that the github page is much better: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e29cb055d108731c8 (It happens that this commit changed quite a lot) > Stef >> >> Uko >> >>> On 05 Sep 2015, at 19:03, Nicolai Hess <[hidden email]> wrote: >>> >>> >>> >>> 2015-09-05 11:48 GMT+02:00 Marcus Denker <[hidden email]>: >>> >>>> On 05 Sep 2015, at 11:40, stepharo <[hidden email]> wrote: >>>> >>>> How can I see the changes? >>>> Our process is not good. Most of us do not get any chance understanding what is changing. >>>> >>> >>> -> download the image before it was added >>> -> merge the slice. >>> >>> Yes, our process is not good… but from a review perspective, this issue is the best we can do. >>> *two* reviews, both from people actively contributing to exactly that part of the system. >>> >>> If we require more, we will be back at a process where due to Fear we do nothing. >>> >>> >>> Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may >>> miss some important relation that only really visible during debugging. >>> I did some test when reviewing this fix. But really, there are so many >>> ways do you need to consider >>> - different ways for opening nautilus >>> - navigate with keys and mouse >>> - selecting updating / changes from other browser and system changes. >>> - refactorings >>> >>> It is really not that easy and I am happy that Franck and Yuri are actually trying to >>> improve Nautilus. >>> you can not easily tests all and make the changes and reviews bullet proof. >>> >>> >>> nicolai >>> >>> >>> >>>> >>>>> Hi, >>>>> >>>>> This is a side effect of >>>>> >>>>> https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcements-for-a-single-action >>>>> >>>>> (which was reviewed by two people, so not obvious). >>>>> >>>>> What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop. >>>>> >>>>> Should be easy to fix for the people involved in case 16475. >>>>> >>>>> Marcus >>>>> >>>>> >>>>> On Sat, Sep 5, 2015 at 8:51 AM, stepharo <[hidden email]> wrote: >>>>> Hi >>>>> >>>>> I do not know if this is linked to recent changes but we cannot remove classes or move them to another package. >>>>> >>>>> Stef >>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> -- >>>>> Marcus Denker -- [hidden email] >>>>> http://www.marcusdenker.de >> > |
Re: ***important*** cannot remove a class anymore...
|
|
git clone https://github.com/pharo-project/pharo-core.git But this is post-integration, we want to review things before they break something, not after. So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list. Also these commits combine several unrelated issues, which is not good for review. Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it? Peter |
Re: ***important*** cannot remove a class anymore...
|
|
> On 06 Sep 2015, at 01:41, Peter Uhnák <[hidden email]> wrote: > > git clone https://github.com/pharo-project/pharo-core.git > cd pharo-core/ > git show 2536752ecdd88e15b00c6d3e29cb055d108731c8 > > But this is post-integration, we want to review things before they break something, not after. > So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list. > > Also these commits combine several unrelated issues, which is not good for review. > > Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it? > > Peter You are right, Peter. But the way the rest of the world (and many people using git from Pharo or other Smalltalk implementations) is that you propose patches which I believe are quite similar to commits. That would be the equivalent of our slice, I guess. Sven |
|
|
In reply to this post by Peter Uhnak
Peter
I just want to get an idea of what is happening. Even after commit. Le 6/9/15 01:41, Peter Uhnák a écrit :
|
|
|
In reply to this post by Sven Van Caekenberghe-2
Le 6/9/15 00:33, Sven Van Caekenberghe a écrit : >> On 05 Sep 2015, at 21:34, stepharo <[hidden email]> wrote: >> >> >> >> Le 5/9/15 19:18, Yuriy Tymchuk a écrit : >>> In fact you couldn’t spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it I’ve spent probably 1.5 hours :) >> sure this is not my point. >> my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. >> I want to see diffs right in front of my nose. > I know very little about git, but the first thing that I tried worked: > > git clone https://github.com/pharo-project/pharo-core.git > cd pharo-core/ > git show 2536752ecdd88e15b00c6d3e29cb055d108731c8 I went to github. I do not want to have to type the hash and this. How do I know that this this hash corresponds to the fixes that have been submitted? Then the output is ***awful***. So if this is that easy why can we have a mail with the diff? Stef > > I assume that we could put that textual information in the mail. > > In my terminal, it is coloured, but I still think that the github page is much better: > > https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e29cb055d108731c8 > > (It happens that this commit changed quite a lot) > >> Stef >>> Uko >>> >>>> On 05 Sep 2015, at 19:03, Nicolai Hess <[hidden email]> wrote: >>>> >>>> >>>> >>>> 2015-09-05 11:48 GMT+02:00 Marcus Denker <[hidden email]>: >>>> >>>>> On 05 Sep 2015, at 11:40, stepharo <[hidden email]> wrote: >>>>> >>>>> How can I see the changes? >>>>> Our process is not good. Most of us do not get any chance understanding what is changing. >>>>> >>>> -> download the image before it was added >>>> -> merge the slice. >>>> >>>> Yes, our process is not good… but from a review perspective, this issue is the best we can do. >>>> *two* reviews, both from people actively contributing to exactly that part of the system. >>>> >>>> If we require more, we will be back at a process where due to Fear we do nothing. >>>> >>>> >>>> Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may >>>> miss some important relation that only really visible during debugging. >>>> I did some test when reviewing this fix. But really, there are so many >>>> ways do you need to consider >>>> - different ways for opening nautilus >>>> - navigate with keys and mouse >>>> - selecting updating / changes from other browser and system changes. >>>> - refactorings >>>> >>>> It is really not that easy and I am happy that Franck and Yuri are actually trying to >>>> improve Nautilus. >>>> you can not easily tests all and make the changes and reviews bullet proof. >>>> >>>> >>>> nicolai >>>> >>>> >>>> >>>>>> Hi, >>>>>> >>>>>> This is a side effect of >>>>>> >>>>>> https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcements-for-a-single-action >>>>>> >>>>>> (which was reviewed by two people, so not obvious). >>>>>> >>>>>> What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop. >>>>>> >>>>>> Should be easy to fix for the people involved in case 16475. >>>>>> >>>>>> Marcus >>>>>> >>>>>> >>>>>> On Sat, Sep 5, 2015 at 8:51 AM, stepharo <[hidden email]> wrote: >>>>>> Hi >>>>>> >>>>>> I do not know if this is linked to recent changes but we cannot remove classes or move them to another package. >>>>>> >>>>>> Stef >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> -- >>>>>> Marcus Denker -- [hidden email] >>>>>> http://www.marcusdenker.de > > |
Re: ***important*** cannot remove a class anymore...
|
|
Hi, There is clearly no downside of having more detailed diff information available. We do not have it now, and the proposal of Stef is more than reasonable. It would be great to have someone willing to spend a bit of time helping him to achieve this. Who would like to give this a try? Cheers, Doru On Sun, Sep 6, 2015 at 10:28 AM, stepharo <[hidden email]> wrote:
|
|
|
In reply to this post by Peter Uhnak
I thought the Squeak email diffs were post-commit?? Now what might be an interesting work flow would be when an issue is tagged Fix Review Needed and the CI monkey validates the issue it pushes the slice to a new IssueNNNNN branch in github and branch diff emailed from there. Cheers ben |
Re: ***important*** cannot remove a class anymore...
|
|
You are right, Peter. But the way the rest of the world (and many people using git from Pharo or other Smalltalk implementations) is that you propose patches which I believe are quite similar to commits. That would be the equivalent of our slice, I guess. I don't think that slice has equivalence in git, because we have changes much more fine-grained. Git patch is simply a diff between two different versions. And even though github (or bitbucket) offer things like "pull requests", they are only a nicer way of presenting a diff. But this doesn't really matter, because the result will be still the same. Now what might be an interesting work flow would be when an issue is tagged Fix Review Needed and the CI monkey validates the issue it pushes the slice to a new IssueNNNNN branch in github and branch diff emailed from there. Unless we want to migrate from fogzbugz to github and do code-reviews there in pull requests there's really no reason to create new branches. You can just create detached commits. (But then again, creating branches is dirt cheap). Actually for a diff you don't need to even commit, because you can compare your current code to the commited one (just like in Monticello). Peter |
Re: ***important*** cannot remove a class anymore...
|
|
But this is all implementation details, I'm still not sure what exactly we (Stef) want to send. * change diff after each successful CI build for an issue? * diff after the change has been integrated? Peter |
«
Return to Pharo Smalltalk Developers
|
1 view|%1 views
| Free forum by Nabble | Edit this page |