filetree MCFileRepositoryInspector>>refresh should be an override

MCFileRepositoryInspector>>refresh should be an override

Open ThierryGoubier opened this issue 10 years ago • 11 comments

MCFileRepositoryInspector>>refresh is defined as belonging to MonticelloGUI (MarcusDenker.213) and in MonticelloFileTree-Core there is a different version.

Should the refresh in MonticelloFileTree-Core be put as an override (i.e. in category *monticellofiletree-core-override instead of *monticellofiletree-core?)

Jul 23 '13 10:07 ThierryGoubier

Good point...however, we really shouldn't have to do the override. If this code:

    versions := OrderedCollection new.
    repository readableFileNames
        do: [ :each | 
              self addVersionInformationExtractedFrom: each to: packageNames ].
    versions := versions select: [ :each | (each at: 3) isNumber ].

were moved to MCRepository>>retrieveVersionsWithPackageNames:, we could drop the override altogether.

This kind of extraction work belongs in the repository code, not the GUI.

Perhaps @demarey could look into having this change made in Pharo3.0?

Jul 23 '13 13:07 dalehenrich

It has my preferences. Once there is an issue for that, I'll add an issue as a backport to 2.0.

I'm not having much success with backporting issues in 2.0 at the moment :(

Jul 23 '13 14:07 ThierryGoubier

In this case, should addVersionInformationExtractedFrom:to: also be moved to MCRepository? It uses an instance variable.

Jul 23 '13 14:07 demarey

I didn't notice the instance variable usage ... it looks like the algorithm could be slightly changed to return a collection of versions instead of setting the versions directly then addVersionInformationExtractedFrom:to: could (and should) be moved to MCRepository ...

I think the last little bit where it filters out versions that are not numbers probably belongs in the inspector (I note that my override of refresh doesn't do that last bit)...

Dale ----- Original Message -----

Jul 23 '13 15:07 dalehenrich

The work is in progress. I moved addVersionInformationExtractedFrom:to: and versionsWithPackageNames: (was retrieveVersionsWithPackageNAme:) to MCFileBasedRepository as versionsWithPackageNames: uses readableFileNames only defined in MCFileBasedRepository. You will need to check if current implementation of retrieveVersionsWithPackageNames: fits FileTree needs.

Aug 22 '13 13:08 demarey

Need to wait for integration into Pharo: https://pharo.fogbugz.com/default.asp?11445

Aug 22 '13 13:08 demarey

I'm not sure what you mean by saying: You will need to check if current implementation of retrieveVersionsWithPackageNames: fits FileTree needs. If the tests pass they meet FileTree's needs:)

Perhaps you have some other question in mind?

If you want me to review some code, I'll do that but it would help if you supplied some code to review (pull request or pointer to a commit) so I can properly make comments...

Aug 22 '13 13:08 dalehenrich

I just mean that default implementation of retrieveVersionsWithPackageNames:is slightly different from the one in FileTree. FileTree:

name last isDigit
                ifFalse: [ 
                    Array
                        with: name
                        with: ''
                        with: ''
                        with: each ]
                ifTrue: [ 
                    Array
                        with: (packageNames add: (name copyUpToLast: $-))
                        with: ((name copyAfterLast: $-) copyUpTo: $.)
                        with: ((name copyAfterLast: $-) copyAfter: $.) asInteger
                        with: each  "pkg name"  "user"  "version" ] ]

default:

name last isDigit
        ifTrue: [ 
            versions
                add:
                    {(name copyUpToLast: $-).   "pkg name"
                    ((name copyAfterLast: $-) copyUpTo: $.).    "user"
                    (((name copyAfterLast: $-) copyAfter: $.) asInteger ifNil: [ 0 ]).  "version"
                    readableFileName }]

Aug 22 '13 14:08 demarey

Ah ... but the one for FileTree is correct for FileTree and the default implementation is correct for Pharo3.0, right?

I apologize if I am misremembering but I don't have all of the code (especially pharo3.0) stuff in my head:)

----- Original Message -----

| I just mean that default implementation of | retrieveVersionsWithPackageNames: is slightly different from the one | in FileTree. | FileTree: name last isDigit | ifFalse: [ | Array | with: name | with: '' | with: '' | with: each ] | ifTrue: [ | Array | with: (packageNames add: (name copyUpToLast: $-)) | with: ((name copyAfterLast: $-) copyUpTo: $.) | with: ((name copyAfterLast: $-) copyAfter: $.) asInteger | with: each "pkg name" "user" "version" ] ] | default: name last isDigit | ifTrue: [ | versions | add: | {(name copyUpToLast: $-). "pkg name" | ((name copyAfterLast: $-) copyUpTo: $.). "user" | (((name copyAfterLast: $-) copyAfter: $.) asInteger ifNil: [ 0 ]). | "version" | readableFileName }] | — | Reply to this email directly or view it on GitHub .

Aug 22 '13 15:08 dalehenrich

Yes, my question was rather: should we remove the implementation of retrieveVersionsWithPackageNames:in filetree? Implementations are not so different, and maybe the default implementation is also fine for FileTree. But maybe not ...

Aug 23 '13 11:08 demarey

@demarey you right now if you submit a pull request with what you are suggesting I would be able to make my judgement ... we would have test results in hand and I would see what you mean by *remove retrieveVersionsWithPackageNames: * .

FileTree implement retrieveVersionsWithPackageNames: in two places one in MCRepository and one in MCFileBasedRepository .. retrieveVersionsWithPackageNames: is called from MCFileRepositoryInspector>>refresh and this is where the override exists ...

If you are suggesting removing the implementation in MCRepository I believe that it does not matter to FileTree. If you are suggesting removing the implementation in MCFileBasedRepository then it does matter to FileTree...

Being able to do anything at all presupposes that retrieveVersionsWithPackageNames: is implemented in the base and called from MCFileRepositoryInspector>>refresh, so I am assuming that you are talking about only doing this for Pharo3.0?

Everything would actually be simpler and clearer if you put together a pull request with all of the changes that you are proposing ... if the tests pass and I don't see anything wrong after a code review, we'll make it so ...

Aug 23 '13 18:08 dalehenrich

filetree filetree copied to clipboard

MCFileRepositoryInspector>>refresh should be an override

filetree
filetree copied to clipboard