Object-centric MetaLink integration. #2120
Conversation
| ] | ||
|
|
||
| { #category : #'*Reflectivity' } | ||
| Object >> link: aMetaLink toAST: aNode [ |
There was a problem hiding this comment.
Can you comment why methods on Object are required?
There was a problem hiding this comment.
In most cases you delegate to target objects which is good. But why this Object methods are good idea?
There was a problem hiding this comment.
The idea is to have an interface directly from objects to scope metalink to them instead of calling complex metalink API.
For instance: anObject link: metalink toAST: aNode
instead of: aNode link: metalink forObject: anObject
Same for classes.
The link:toAST: is the most generic interface that does not make assumptions on the kind of node it must install the link.
There was a problem hiding this comment.
I see. I misunderstood it originally.
So anObject here is where metalink will be active. For other receivers metalink will be not executed for given ast-node. Right?
|
Steven can you have a look because some failing tests are |
Where can i see the failing test, because in jenkins i do not see it and the test is green on my machine :'( |
|
Click on Details (above) then on the Test in the "menu bar" |
|
Thanks. Ok this is because of remaining metalinks on breakpoint test classes which are created and removed (by the test) from the system. Somehow metalinks are not uninstalled from those classes. See BreakpointTest>>newDummyClass. I will look at it. |
|
I have the impression it's too late for Pharo 7. It should probably be rebased in Pharo 8. |
|
OK. How do i do that? Note that the problem is not in this code, but is due to a metalink bug already in Pharo 7 (I need to report that bug...). |
|
There is now a "Pharo7.0" branch and additionally also a "Pharo8.0" branch. You need to do a PR agains the P8 one. |
|
Not even that :)
In your PR you have a button “Edit” that allows you to change the destination.
Esteban
… On 3 Jan 2019, at 13:05, Astares ***@***.***> wrote:
There is now a "Pharo7.0" branch and additionally also a "Pharo8.0" branch. You need to do a PR agains the P8 one.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Should i do that now or can it wait next week? I would like to check with Marcus, but if this is urgent i can edit that today. |
|
I do not see if this is just a better API to reflectivity why this cannot be added to P7? |
|
Well all the PR code is in the "Reflectivity bubble", and should not affect the rest of the system. But three tests are failing because of a bug triggered by breakpoint tests executed before, for which i can provide a temporary fix in my code but i need to talk to Marcus for fixing the real bug - so it would have to wait next week... I will submit a bug today and a mailing list question. Thing is i understood it seemed to be rather urgent... |
…ey were removed. In that case it tries to find a method which does not exists when uninstalling the link. The impact of this change is limited to Reflectivity, specifically the uninstallation of remaining links on obsolete classes.
…d that does not exist in the class on which it is supposed to be (class made obsolete with niled method dictionary)
|
There are no more errors in tests related to Reflectivity. |
|
Still it's strange that there are 12 broken tests, when the pharo build is mostly green. |
|
Also, if this is for Pharo7 you should open an issue in fogbugz related to it |
|
Hi @StevenCostiou does this have a related issue in fogbugz. How we can tested if it is working? |
|
Yes, this is fogbugz 22830. There are no new packages, only update of the Reflectivity packages. The code can be loaded from this branch https://github.com/StevenCostiou/pharo/tree/22830-Object-centric-metalink-integration but tests are passing. I will try again a full Pharo test on my image. |
|
Failing tests are unrelated! |
Tests also added. Everything should be separated from the core, so features should be within their own private bubble.