Manifest class values - instance or class side
It is unclear if literal values in Manifest classes should be on the instance side or on the class side. demarey wrote in the package comments thread on May 6th: "Data is stored in raw strings in methods (instance side) of the Manifest." However, when I look in the actual manifest classes, it seems like the manifest values (from the rule checker) are stored in the class side. In our work on package comments, we want to put the package comments in the manifest classes, but we are uncertain if it should be on the instance or class side. -- View this message in context: http://forum.world.st/Manifest-class-values-instance-or-class-side-tp4826443... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
Kasper Osterbye wrote
It is unclear if literal values in Manifest classes should be on the instance side or on the class side
I noticed the conflicting info also and was wondering the same thing :) ----- Cheers, Sean -- View this message in context: http://forum.world.st/Manifest-class-values-instance-or-class-side-tp4826443... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
I think the ambiguity comes from the fact that a manifest, when modelled as a class, es a singleton. So it really doesn't matter where the methods are; they will work. If this where modelled properly, a single class which would hold the Package data, then we would have only one class (Package) and several instances, each with its own variables: author, comment, creationDate, etc. So for now, maybe it's irrelevant the distinction. Because where the methods are do not really reflect anything on the reality we are modelling.
Having said that, maybe we should strive for consistency to facilitate the next iteration: The refactoring which will convert one-class per Package to one-object per Package. â
Class side because we do not want to create instances of manifest. Stef Le 15/5/15 07:30, Kasper Osterbye a écrit :
It is unclear if literal values in Manifest classes should be on the instance side or on the class side. demarey wrote in the package comments thread on May 6th: "Data is stored in raw strings in methods (instance side) of the Manifest."
However, when I look in the actual manifest classes, it seems like the manifest values (from the rule checker) are stored in the class side.
In our work on package comments, we want to put the package comments in the manifest classes, but we are uncertain if it should be on the instance or class side.
-- View this message in context: http://forum.world.st/Manifest-class-values-instance-or-class-side-tp4826443... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
âWhat about this method? RPackage>>packageManifest ^ (self classes detect: [ :each | each isManifest ] ifNone: [ TheManifestBuilder new createManifestNamed: name]) *new* Anyone know why does it create an instance of the Manifest instead of returning the class? (or why are we using instances of Manifests in the image?)
Sergio Fedi wrote
âWhat about this method?
RPackage>>packageManifest ^ (self classes detect: [ :each | each isManifest ] ifNone: [ TheManifestBuilder new createManifestNamed: name]) *new*
Anyone know why does it create an instance of the Manifest instead of returning the class? (or why are we using instances of Manifests in the image?)
Perhaps it should be filed as a bug? -- View this message in context: http://forum.world.st/Manifest-class-values-instance-or-class-side-tp4826443... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
Le 15/5/15 22:48, Sergio Fedi a écrit :
âWhat about this method?
RPackage>>packageManifest ^ (self classes detect: [ :each | each isManifest ] ifNone: [ TheManifestBuilder new createManifestNamed: name]) *new*
Anyone know why does it create an instance of the Manifest instead of returning the class?
Did you check that TheManifestBuilder do not return a class?
(or why are we using instances of Manifests in the image?)
createManifestNamed: packageName manifestClass := PackageManifest subclass: (self class manifestTag, packageName onlyLetters) asSymbol instanceVariableNames: '' classVariableNames: '' poolDictionaries: '' category: packageName. manifestClass comment: self class manifestClassComment. ^ manifestClass so this is probably a bug, RPackage>>packageManifest ^ (self classes detect: [ :each | each isManifest ] ifNone: [ TheManifestBuilder new createManifestNamed: name]) new Can you fill up an bug entry? Tx
Aye, I will. Although we already changed it in our PackageComments fix. But it's probably better to resolve ir separately as a different case. On the same line, I noted that the PackageManifest and ManifestKernel have instance methods (rejectClasses, rejectRules and ruleConsistencyCheckRuleV1FalsePositive) But those don't make sense if you always use the class. I don't know if that's an issue right now, but I'm pointing it out just in case. PD: I'm having trouble with the site (https://pharo.fogbugz.com/f/) right now. I'll create and resolve it later.
I want to add a test to ensure #packageManifest answers a Class. Since RPackage is in RPackageCore, but the #packageManifest method is in Manifest-Core I'm not sure whether: 1. I have to make the test in RPackageTest but make the test method from the Manifest-Core package 2. I have to make the test in RPackageTest but make the test method from the Manifest-Tests package 3. Create a new class in Manifest-Tests to hold this new test (and someupcoming tests as well) â
On Mon, May 18, 2015 at 10:02 AM, Sergio Fedi <sergio.fedi@gmail.com> wrote:
I want to add a test to ensure #packageManifest answers a Class.
Since RPackage is in RPackageCore, but the #packageManifest method is in Manifest-Core I'm not sure whether:
1. I have to make the test in RPackageTest but make the test method from the Manifest-Core package 2. I have to make the test in RPackageTest but make the test method from the Manifest-Tests package 3. Create a new class in Manifest-Tests to hold this new test (and someupcoming tests as well)
I'm not sure, but I am tending to towards... use of Manifest is an implementation detail. Its RPackage behaviour you are testing, so that is where the tests should
Supposing we do that, that we put these tests in RPackageTests, What tests de we put in Manifest-Tests?
Ok.
From what you say I get the impression that maybe all the package Manifests is modelling the hack of adding metadata to RPackage.
In that case, formally, we should be testing RPackage functionality. But since the hack is so semantically large that it must be held in a package, we end up having tests in that package for testing that functionality.
Case created:
https://pharo.fogbugz.com/f/cases/15557/New-PackageManifests-should-be-class...
Case Resolved (Review needed)
Sergio Fedi wrote
Case created:
https://pharo.fogbugz.com/f/cases/15557/New-PackageManifests-should-be-class...
Case Resolved (Review needed)
It is scary, but all pharo unittests succeed with either implementation :-( -- View this message in context: http://forum.world.st/Manifest-class-values-instance-or-class-side-tp4826443... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
Why scary? It seems that it was a part of the system not used. Maybe the scary part was to have something implemented that wasn't used?
Le 15 mai 2015 à 07:30, Kasper Osterbye a écrit :
It is unclear if literal values in Manifest classes should be on the instance side or on the class side. demarey wrote in the package comments thread on May 6th: "Data is stored in raw strings in methods (instance side) of the Manifest."
However, when I look in the actual manifest classes, it seems like the manifest values (from the rule checker) are stored in the class side.
In our work on package comments, we want to put the package comments in the manifest classes, but we are uncertain if it should be on the instance or class side.
sorry for the late reply. Indeed data used to be stored in class side of Manifests. I changed this behavior to be able to easily inspect a Manifest through the inspector. Getting back to have them on class side is not really a problem. I must admit I don't like to have classes you never instantiate but Manifest classes are just a hack to be able to store meta-dat with Monticello.
participants (6)
-
Ben Coman -
Christophe Demarey -
Kasper Osterbye -
Sean P. DeNigris -
Sergio Fedi -
stepharo