Smalltalk › Squeak › Squeak - Dev

The Inbox: Monticello-nice.734.mcz

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

commits-2

The Inbox: Monticello-nice.734.mcz

A new version of Monticello was added to project The Inbox:
http://source.squeak.org/inbox/Monticello-nice.734.mcz

==================== Summary ====================

Name: Monticello-nice.734
Author: nice
Time: 30 December 2020, 3:57:21.60758 pm
UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
Ancestors: Monticello-mt.733

Workaround version history browser bug when 'working copy' is selected.

The snapshot is searched in repositoryGroup, and logically, none is found.
Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.

The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).

If you can think of nicer fix, please raise voice.

=============== Diff against Monticello-mt.733 ===============

Item was changed:
----- Method: MCVersionHistoryBrowser>>selectedSnapshot (in category 'accessing') -----
selectedSnapshot
+ index = 1 ifTrue: [^package snapshot].
^ self snapshotForInfo: self selectedInfo!

David T. Lewis

Re: The Inbox: Monticello-nice.734.mcz

On Wed, Dec 30, 2020 at 02:57:23PM +0000, [hidden email] wrote:

> A new version of Monticello was added to project The Inbox:
> http://source.squeak.org/inbox/Monticello-nice.734.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-nice.734
> Author: nice
> Time: 30 December 2020, 3:57:21.60758 pm
> UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
> Ancestors: Monticello-mt.733
>
> Workaround version history browser bug when 'working copy' is selected.
>
> The snapshot is searched in repositoryGroup, and logically, none is found.
> Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
> This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.
>
> The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).
>
> If you can think of nicer fix, please raise voice.
>

Attached is another way to do it without relying on the list index. I have
not tested very much but it seems to be all right.

MCVersionHistoryBrowser>>snapshotForInfo: aVersionInfo
"Answer snapshot for aVersionInfo or for the working copy if not found"
^ (self repositoryGroup
versionWithInfo: aVersionInfo
ifNone: [ package ]) snapshot

Apologies for violating MC package standards by including a method comment ;-)

Dave

MCVersionHistoryBrowser-snapshotForInfo.st (373 bytes) Download Attachment

Nicolas Cellier

Re: The Inbox: Monticello-nice.734.mcz

Yeah, no comment nowhere and most methods 'as yet unclassified' (we fixed many of those already).

I guess that the idea was that with a clear separation of concerns and limitation of responsibilities, comments coud eventually become moot.

Even classification of methods could be seen as unhelpful if there are few enough per class...

It's been a nice exercise in style.

But in corners like this, nothing is that obvious, sorry but we're well beyond the limit of what an average Smalltalker can follow and understand...

The alternative you're proposing does work thanks to an ugly ifError: handling...

We're sending versionName to a MCWorkingCopyAncestry which is a MNU.

We have absolutely no idea why a MCWorkingCopyAncestry would ever be a good candidate for self selectedInfo...

We have no idea why a MCVersionInfo has to be a specialization of a MCAncestry.

But then, should we subclass MCWorkingCopyAncestry and get a MCWorkingCopyInfo? Inheritance vs composition?

We have no idea why we must search a specific version in MCRepositoryGroup default, rather than the more restricted group registered for this package...

So we end up scanning all the repositories in the default repositoryGroup for getting a snapshot of working copy, which is quiet a vain idea, the snapshot lies in no repository, it's right there under our mouse, in the image.

Clearly not such a good example of crystal clear and straightforward code to my eyes...

I'd rather say crooked!

I don't know when and how exactly we got there...

Is it from inception? Or gradual rot due to quick hacking (we often do the EASIEST thing that could possibly work, rather than the SIMPLEST thing that could possibly sustain)...

But at this stage, we need more than comments. We need deeper clean up.

Every piece of code requires such lifting in order to sustain the burden of age, even the nicest pieces...

Concerning the lack of comments, I have my own opinion: I think it's a great mistake.

The mental image of the zoo of classes and concepts is certainly obvious to the authors, or the experts, once sufficiently fluent.

But the lack of comment or tutorial or even the lightest introduction to the concepts and key implementation choices, will for sure delay the learning curve of newbies.

If you have to deal with such a bug, you're directly thrown into the crooked parts of code, and the austerity of comment is just intimidating. Where to begin the journey in MC?

Let's follow my example: why does each and every MCWorkingCopy has a (nil) versionInfo?

My guess when reading code is that such info could be (have been?) a proxy for rebuilding the ancestry (a cheaper memory footprint?).

But then what? There no setter but nil... Is it a dead code?

When the bug I'm after concerns the selectedInfo of the working copy, the lack of comment and clear intentions just let me lost in a desert.

My idea would be to (double) dispatch the search of snapshot to the info, but I have no idea what could be a good candidate implementation.

End of rant!

Le mer. 30 déc. 2020 à 17:52, David T. Lewis <[hidden email]> a écrit :

On Wed, Dec 30, 2020 at 02:57:23PM +0000, [hidden email] wrote:
> A new version of Monticello was added to project The Inbox:
> http://source.squeak.org/inbox/Monticello-nice.734.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-nice.734
> Author: nice
> Time: 30 December 2020, 3:57:21.60758 pm
> UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
> Ancestors: Monticello-mt.733
>
> Workaround version history browser bug when 'working copy' is selected.
>
> The snapshot is searched in repositoryGroup, and logically, none is found.
> Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
> This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.
>
> The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).
>
> If you can think of nicer fix, please raise voice.
>

Attached is another way to do it without relying on the list index. I have
not tested very much but it seems to be all right.

MCVersionHistoryBrowser>>snapshotForInfo: aVersionInfo
"Answer snapshot for aVersionInfo or for the working copy if not found"
^ (self repositoryGroup
versionWithInfo: aVersionInfo
ifNone: [ package ]) snapshot

Apologies for violating MC package standards by including a method comment ;-)

Dave

timrowledge

Re: The Inbox: Monticello-nice.734.mcz

> On 2020-12-30, at 10:11 AM, Nicolas Cellier <[hidden email]> wrote:
[eloquent rant elided]

I refer us, as so often, to Adele's Dictum: "If it isn't documented, it doesn't exist. If it doesn't exist, what exactly did we pay you for?"

Of course, part of the problem is that for a lot of things surprisingly important to us *we didn't pay anyone*.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Thesaurus: Ancient reptile with a truly extensive vocabulary

marcel.taeumel

Re: The Inbox: Monticello-nice.734.mcz

In reply to this post by commits-2

Hi Nicolas.

> If you can think of nicer fix, please raise voice.

I just put Monticello-mt.734 into the inbox. Please take a look and decide for yourself. :-)

Best,

Marcel

Am 30.12.2020 15:57:30 schrieb [hidden email] <[hidden email]>:
A new version of Monticello was added to project The Inbox:
http://source.squeak.org/inbox/Monticello-nice.734.mcz

==================== Summary ====================

Name: Monticello-nice.734
Author: nice
Time: 30 December 2020, 3:57:21.60758 pm
UUID: 863f642a-7795-4eb6-aa6b-76a5af3a8504
Ancestors: Monticello-mt.733

Workaround version history browser bug when 'working copy' is selected.

The snapshot is searched in repositoryGroup, and logically, none is found.
Note that the self selectedInfo isKindOf: MCWorkingCopyAncestry, which does not behave as a regular MCVersionInfo, it does not even answer to versionName...
This somehow accelerate the failure thru an ifError: [] handling, but at the end, the selectedSnapshot is still nil which makes the 'view changes from...' menu fail.

The workaround is not very nice, it use implicit knowledge that 'working copy' will be at top of list (index = 1).

If you can think of nicer fix, please raise voice.

=============== Diff against Monticello-mt.733 ===============

Item was changed:
----- Method: MCVersionHistoryBrowser>>selectedSnapshot (in category 'accessing') -----
selectedSnapshot
+ index = 1 ifTrue: [^package snapshot].
^ self snapshotForInfo: self selectedInfo!