Bugzilla – Full Text Bug Listing
| Summary: | Support initial values for properties | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Kamesh Sampath <kamesh.sampath> | ||||||||||||||||||||||||||||||||||||||||
| Component: | Sapphire | Assignee: | Konstantin Komissarchik <konstantin> | ||||||||||||||||||||||||||||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||||||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | kamesh.sampath, konstantin | ||||||||||||||||||||||||||||||||||||||||
| Version: | unspecified | Keywords: | plan | ||||||||||||||||||||||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||
|
Description
Kamesh Sampath
Thinking about this problem, I think it would be better to create a new annotation like @InitialValue rather than overloading the semantics of existing @DefaultValue annotation with @Mandatory. @InitialValue would need a corresponding InitialValueService (see @DefaultValue/DefaultValueService). The existing standard XML binding facilities for list and element properties should be modified to respect @InitialValue annotation. It sounds like you would be interested in contributing this enhancement. Since this is a fairly small item, I am going ahead and setting the target to 0.4 release. I am assigning the bug to myself simply because Eclipse development process requires bugs to be assigned to committers, but this is still your item to develop unless I hear otherwise. Yeah I can take care of it and will start working on it. But then how to start with putting the fix i.e 1. checkout the code using CVS or Git 2. Applying the fix and testing it ? Do we have any testing mechanisms in place for it ? 3. Commit the code back and push it to eclipse stream etc., Also what requires it to be Eclipse committer ? ~Kamesh Sorry forgot to mention what Standard Eclipse platform to use ? Take a look at the following page, especially "Getting Started Guide for Contributors". http://www.eclipse.org/sapphire/developers/index.php > 1. checkout the code using CVS or Git Sapphire is in CVS. See above link for details. > 2. Applying the fix and testing it ? Do we have any testing mechanisms in place > for it ? There is a test plugin that you will find in CVS. You can run the top-level test suite locally in your dev workspace. Alternatively, if you've followed the building locally instructions, you can call "ant hudson-build" which will simulate what would happen if this change was committed, including running all the tests. > 3. Commit the code back and push it to eclipse stream etc., Per Eclipse process, as a contributor, you would create a patch (right click on a project and select Team->Create Patch). You would then attach the patch to this bug. Once I've reviewed and deemed the patch ok, I will commit it into CVS. > Also what requires it to be Eclipse committer ? You can become a committer on Sapphire by showing ongoing track record of quality contributions (patches). The key is to demonstrate both understanding of the codebase (minimal to no changes required on an initial patch) and ongoing commitment (evidence of participation over a length of time - as opposed to a short spurt of activity and then silence). After that, there are some legal aspects that need to be cleared (like a formal ok from your employer). > Sorry forgot to mention what Standard Eclipse platform to use ?
The best thing to do is to follow instructions on building locally on the link that I posted previously. A local build will create dev-target eclipse installation that is exactly equivalent to what the production build will use. Absent that, you can substitute Eclipse Indigo (Java EE edition) package as your target.
I have followed the instructions @ http://www.eclipse.org/sapphire/developers/index.php and now i have dev-eclipse, dev-target and build/repository folders under my CVS sources folder. 1. Which one i need to take for my patch development dev-eclipse or dev-target ? 2. How to import the source in to my work space, just the normal process, start eclipse from one of the dev-eclipse or dev-target and from within your package explorer give "Import Plugins and Fragments", select the $SOURCE_HOME/build/repository/plugins then its the usual process of doing build and development Please correct me if I have missed somethig ~Kamesh Continuing from my previous comment I am using the dev-eclipse and installed the Sapphire SDK from the build/repository and checked out the code from CVS Now my confusion how am i suppose to have them as individual Plugin Projects ? Just by doing Import Plugins and Fragments stuff ? > Which one i need to take for my patch development dev-eclipse or dev-target dev-eclipse is an eclipse installation tuned for development of Sapphire. It includes Sapphire SDK. dev-target is what you should use as the target platform. > 2. How to import the source in to my work space, just the normal process, start > eclipse from one of the dev-eclipse or dev-target and from within your package > explorer give "Import Plugins and Fragments", select the > $SOURCE_HOME/build/repository/plugins Right. Import plugins directly into your workspace from the above folder. Note that you don't even need to import every plugin if you've set dev-target as your target platform. Just the ones you want to work on. The import thing to do is to configure CVS repository for Sapphire in CVS repositories view before you import the projects. That should ensure that Eclipse is aware that projects are connected to CVS so that it can help you generate the patch when time comes. 1. I have set dev-eclipse and dev-target as said above. 2. I have create the CVS repository view to point to repository :pserver:anonymous@dev.eclipse.org:/cvsroot/technology 3. When I try to do the "Import Plugins and Fragments" from the $SOURCE_HOME/build/repository/plugins i get have the projects imported and they are not 3.1 JDT enabled - I mean am not able to edit the souce code using JDT 3.2 How to i know that its connected to CVS or not ? I dont see the CVS label decorations on the projects 4. Instead of #3 shall i checkout out the code from :pserver:anonymous@dev.eclipse.org:/cvsroot/technology/org.eclipse.sapphire ? Even then I have problems enabling JDT for the project and seeing projects plugins as individual projects in Eclipse workspace Please let me know what I am missing here to make things clear and staright for me to start working on the task > 3. When I try to do the "Import Plugins and Fragments" from the
> $SOURCE_HOME/build/repository/plugins
My fault for not spotting that earlier. You don't import from build/repository directory. Your import from $SOURCE_HOME/plugins.
yeah now I see i just did the normal import of the projects from the $SOURCE_HOME/plugins, it just imported them and i see the CVS Repo labels on them and JDT enabled on it. One last question to keep it synchronized with the eclipse rep, i trigged the synchronize but i see that couple of projects org.eclipse.sapphire.modeling and org.eclipse.sapphire.modeling.xml are showing outgoing syncs on folder .javacc do i need to add this to my CVS ignore list ? > One last question to keep it synchronized with the eclipse rep, i trigged the
> synchronize but i see that couple of projects org.eclipse.sapphire.modeling and
> org.eclipse.sapphire.modeling.xml are showing outgoing syncs on folder .javacc
> do i need to add this to my CVS ignore list ?
Yep. I haven't been able to figure out how to configure the projects to avoid this problem.
even when i add it to cvsignore its still showing as outgoing changes, i guess the easiset solution is to skip that during patch creation :) one thing i tired and worked, is that i clicked the properties of the folder and chose "CVS" and said Disconnect - only for that folder I have started to work on this Task, can you pelase change the status from "NEW" to ASSIGNED ? sorry forgot one more thing, is there any standard code tempaltes, formatter.xml etc. which i might need to use with my dev environment? > is there any standard code tempaltes, formatter.xml etc. which i might need to
> use with my dev environment?
Set tabs to four space chracters, but outside of that you can use whatever format you like for new files you create. For files you modify, do maintain the existing format.
There is a header that must appear at the top of every file. Make sure to grab a header from an existing file when you create a new file. Adjust copyright and attribution accordingly. There should also be an @author tag in every class.
BTW... I see that you've registered in bugzilla with a personal e-mail address, but from source that you posted on the forum, it looks like you are a liferay employee. Would it be possible for you to change your bugzilla account to use your corporate e-mail address instead? You can do that in account settings (no need to create a new account). That would make accounting that Eclipse Foundation requires easier when it comes time to accepting your patches. hey konstantin, i dont work for liferay I am a Sr.Technical Architect at Accenture, India. the code piece you saw for the contribution am making for the Liferay IDE hence use the package names like that. I am not sure that I can use my official id as am in the process for getting formal approvals, once thats done I will update my bugzilla account. Hi Konstantin, I have fixed the @InitialValue and @InitialValueService, am attaching the context as part of this comment for your review and let me know if my fixes are correct. Also I want to test the fix is there anyway we could do it ? Let me know the process. Once i test it I will submit the final patch. Created attachment 201300 [details]
mylyn/context/zip
i found the test project and saw few test cases and suites like org.eclipse.sapphire.tests.modeling.xml.binding.t000X packages and test cases under them , i guess i need to write one such for this fix too. Let me know if my understanding is right, anything else needs to be done for successful testing? > hey konstantin, i dont work for liferay I am a Sr.Technical Architect at > Accenture, India. the code piece you saw for the contribution am making for the > Liferay IDE hence use the package names like that. I am not sure that I can use > my official id as am in the process for getting formal approvals, once thats > done I will update my bugzilla account. Thanks for clarifying that. Given your current situation, you can contribute up to 250 lines at a time without going through the full Eclipse.org IP process. If you accumulate sufficient amount of contribution to be nominated as a comitter, Accenture will need to sign Member Committer Agreement. http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf > I have fixed the @InitialValue and @InitialValueService, am attaching the > context as part of this comment for your review and let me know if my fixes are > correct. You need to attach a patch file. The attached mylyn context zip isn't for source code sharing. To export a patch, select project or projects that you have changed, then right click -> Team -> Create Patch. > i found the test project and saw few test cases and suites like > org.eclipse.sapphire.tests.modeling.xml.binding.t000X packages and test cases > under them , i guess i need to write one such for this fix too. > > Let me know if my understanding is right, anything else needs to be done for > successful testing? You found the test plugin. Go ahead and create a new numbered test package under xml binding suite. Once your own test is running correctly, you can run the entire test suite (org.eclipse.sapphire.tests.SapphireTestSuite) to look for regressions. Beyond that, we will need content for the enhancements doc for 0.4 (see org.eclipse.sapphire.doc/html/releases/0.4/enhancements.html) and ideally demonstration of this feature in the gallery sample (probably create a new section for initial value support). Created attachment 201371 [details] Modelling Project Patches - Bug 354276 The patch file for org.eclipse.sapphire.modelling for your review Created attachment 201372 [details] Modelling Xml Project Patches - Bug354276 Modelling Xml Project Patches - Bug 354276 Created attachment 201373 [details] Junit Test cases to Test -Bug 354276 Added junit test # 0011 for testing the fixes for Nug-354276 I should have made these patches as one single zip file, not sure it will upload its as individual comments, i will try to make it much simpler next time when I attach the patches. The patches has the fixes for Bug-354276 and you can review them let me know if you want me to make any fixes on top of it. Right now i have one open issue : the serualizer is not creating just an empty element e.g. <name/> when i dont specify @DefaultValue and leave @InitialValue has empty or null. I tried putting this fix in serveral palces but i vain :( I have two thoughts on the same, 1. Providing compile team feedback (error markers) to the user when he/she uses @InitialValue (with no value specified) and even @DefaultValue not added 2. Create empty element e.g <name/> when user is using @InitialValue (with no value specified) and even @DefaultValue not added I prefer to go with the #2 as in some cases the XSD/DTD structure mandates us to have empty element (when no value) when it is sequenced Please share your thoughts. Also if you feel we can have some quick chat over skype or anyting i am ok to do that. If you multi-select all the projects that you've changed and then do export patch, a single patch will contain all the changes. That's better than a zip since bugzilla has patch display facility that will not work with a zip.
1. I don't think the semantics of DefaultValue and InitialValue should mix. It should be possible to have initial value different from default value. Also, the InitialValue.value() attribute should be renamed as InitialValue.text() and made a required attribute. Using magic "value" attribute to achieve compact annotation syntax is bad for future API evolution and I am avoiding that pattern in Sapphire.
2. It looks like you accidentally re-formated MiscUtil and other classes. Make sure not to invoke re-format actions when editing an existing class.
3. Per #1, changes to MiscUtil shouldn't be necessary anyway.
4. The headers on new files are mangled and incorrect. The copyright holder isn't IBM. It's your employer. The "initial API and implementation" line should credit you (full name). Similarly, the @author tags need full name and e-mail address.
5. Let's rename InitialValueService.getInitialValue() to InitialValueService.text(). I am slowly moving to reducing API verbosity across the framework.
6. I know you based InitialValueServiceFactory on DefaultValueServiceFactory, but that's actually an older pattern for implementing services than what I like to use now. See StandardPropertyDocumentationService for a newer pattern. I would expect to see StandardInitialValueService class with an inner Factory class.
7. It looks like applicable condition is only checking that the property is a value property. I think we also should check here if the property has an @InitialValue annotation. This will enable use to use presence/absence of InitialValueService in the initial value setting logic.
8. I don't see registration of the service (no changes to sapphire-extension.xml file).
9. Thinking about this feature some more, I no longer think it is appropriate to handle the initial value setting in the XML binding layer. This feature is more generic and relevant outside of that context. A better and simpler placer to implement setting of the initial value is in ModelElementType.instantiate() method. This should remove the need to patch the XML plugin.
> Right now i have one open issue : the serualizer is not creating just an empty
> element e.g. <name/> when i dont specify @DefaultValue and leave @InitialValue
> has empty or null. I tried putting this fix in serveral palces but i vain :(
>
> I have two thoughts on the same,
>
> 1. Providing compile team feedback (error markers) to the user when he/she
> uses @InitialValue (with no value specified) and even @DefaultValue not added
>
> 2. Create empty element e.g <name/> when user is using @InitialValue (with no
> value specified) and even @DefaultValue not added
>
> I prefer to go with the #2 as in some cases the XSD/DTD structure mandates us
> to have empty element (when no value) when it is sequenced
I believe that empty initial element usecase can be achieved via following means:
@InitialValue( text = "" )
@XmlValueBinding( path = "name", removeNodeOnSetIfNull = false )
The first annotation will tell the framework that the initial value should be assigned to this property (as an empty string) while the second annotation tells the XML binding layer that the bound element should be retained even if its content is empty.
Thanks for your comments, i will revert my changes and rework on your suggestions. 1.I am not understanding the implication of the Service Class and its Factory, 1.1. Who will call this? 1.2. From where ? 1.3. Whats the implication of this class and its factory and what it needs to do ? 4. I am not getting how ModelElementType.instantiate() could provide this implementation ? " my perception here is that from the ModelElementType.instantiate() you want to get the Service class which is of type StandardInitialValueService and get the initialValue from there and set it to the modelProperty which has this annotation" I am sorry for asking these questions, this will make me understand the framework better and give an correct implementation The service is the API by which the framework will retrieve initial value for the property. That is, the only thing that will read the @InitialValue annotation is StandardInitialValueService. It's factory is registered globally in sapphire-extension.xml file in the modeling plugin.
Framework adopter could supply other implementations of InitialValueService either globally or more likely at property-level using @Service annotation. This could be useful if a static initial value isn't sufficient.
In ModelElementType.instantiate(), you would do something like this:
for each value property
{
InitialValueService service = element.service( property, InitialValueService.class );
if( service != null )
{
element.write( property, service.text() );
}
}
Maybe a bit more to traverse into implied element properties, but that should give you an idea.
Created attachment 201424 [details]
Patch v2
Hi Konstantin,
I have reworked on my changes in accordance with you comments above , and attached the patch for your review. Let me know if you are fine with it. But my test cases are failing because of the model not getting serialized, am for sure know that it could be the problem @ ModelElementType.instantiate() , as per your comment i tried moving the initital value setting from XML Binding to ModelElementType
~Kamesh
Created attachment 201425 [details]
mylyn/context/zip
finally i made the serialization working and its serializing the items with our new implementation. There is some glitch and i will sort it out. Now after yor explanation i understand how it works :) now my eyes are teasing and am retiring to bed! have a wonderful weekend! 1. In sapphire-extension.xml, the service id isn't valid. It should be Sapphire.InitialValueService.Standard (Sapphire.[ServiceType].[Variety] convention). The description is also incorrect. There is no connection to a FactsService in this registration. We may want to supply facts about initial value, but that would be a separate service implementing FactsService API. 2. I cannot read the diff for ModelElementType. Whatever you are doing is causing the file to get re-formatted. Do you have re-format on save enabled? Please stop doing that. 3. You've added a whole bunch of copyright headers to files unrelated to your change. Please don't use automated copyright header tools or at least review what the tool has done. 4. StandardInitialValueService should not have getValueProperty/setValueProperty API nor should the property be passed into the constructor. The logic in Factory.create should really be in StandardInitialValueService.init() method. The constructor shouldn't need to take any arguments. To access the property inside service implementation use context( ValueProperty.class ). 5. StandardInitialValueService.create has now unnecessary branch for handling the case where @InitialValue doesn't exist. This case cannot come up due to logic in applicable. 6. Copyright headers are still corrupted in the test plugin changes. You may want to review your patch after exporting to make sure that no extraneous changes are included. #1. taken care and you will see it in next patch Patch v3. # 3. Do you have the files list for which it has been changed, i will try to revert them and make the synched back to Repository #6 I double checked it now i dont see that i have enabled the auto - formatting, but i have an habit of formatting the code after every save which has been my instinct since i have strated to use eclipse, I need to get rid of that habit - atleast editing others file #2 not sure about it, anyway i have revereted it to repository revision and will update it carefully without reformatting #5 and #4 taken care, actually i made the removal of value property once i sent the patch to you and reworte the code ;) > # 3. Do you have the files list for which it has been changed, i will try to
> revert them and make the synched back to Repository
Click on the diff link next to the patch attachment in bugzilla and it will be quite obvious.
no sure how they got changed, I have reverted completely, at the most the following are the files that will be impacted for this change, 1. ModelElementType 2. InitialValue - Annotation 3. StandardIntialValueService 4. Tests - XmlBindingTestSuite, binding.t0011 package classes and txt files I will ensure that when I send the v3 patch i will check files list and send it. Sorry for the inconvinence caused :( Created attachment 201454 [details]
Patch v3
I have reworked on all the classes and identified the only ones to create a new patch that complies with your comments.
Please validate
Open Issue:
I am able to generate the initial value for normal valued properties but when it goes to list properties am not able to get it working, i tried all possiblities but in vain, the pain points are how do I get the model element from List property rather any Model property, because me not getting ModelElement the Service is not created for List Property Children which are again "ValueProperty"
Please advice.
Created attachment 201455 [details]
mylyn/context/zip
1. ModelElementType was still re-formatted beyond the scope of your changes.
2. The changes to ModelElementType are overly complex. There is no reason for two passes (initialValueProperties/writeInitialValues), a queue, etc. All that's needed is one simple loop at the end of instantiate. I am looking for about 10-20 lines of code.
3. The XmlBindingTestSuite header is corrupted. The headers on the new test files are almost right, but are missing the bottom line.
> I am able to generate the initial value for normal valued properties but when
> it goes to list properties am not able to get it working, i tried all
> possiblities but in vain, the pain points are how do I get the model element
> from List property rather any Model property, because me not getting
> ModelElement the Service is not created for List Property Children which are
> again "ValueProperty"
The simple API that's designed for initial value support does not handle specifying creation of child model elements (list and element properties). To do that would be quite complex, so I would suggest focusing this support on ValueProperty and ImpliedElementProperty (which you can traverse into without creating a child element). If you find that not being able to specifying initial contents of a list to be a critical limiting factor, I would suggest handling that evolution as a second enhancement that can be worked on separately once this is completed.
i will reformat only the lines that I edit hereafter and infact reformatted ModelElementType as before
Coming back to business
the following are the lines i have now in ModelElementType
for (ModelProperty modelProperty : this.properties) {
if (modelProperty instanceof ValueProperty) {
ValueProperty tempValueProperty = (ValueProperty) modelProperty;
StandardInitialValueService service = modelElement
.service(tempValueProperty,
StandardInitialValueService.class);
modelElement.write(tempValueProperty,
service.getInitialValue());
} else if (modelProperty instanceof ImpliedElementProperty) {
ImpliedElementProperty tempImpliedProperty = (ImpliedElementProperty) modelProperty;
ModelProperty baseModelProperty = tempImpliedProperty
.getBase();
StandardInitialValueService service = modelElement
.service(baseModelProperty,
StandardInitialValueService.class);
if (baseModelProperty instanceof ValueProperty) {
modelElement.write(
(ValueProperty) baseModelProperty,
service.getInitialValue());
}
} else {
throw new UnsupportedOperationException(
getSimpleName()
+ " does not support @InitialValue on List properties");
}
}
I know the ImpliedElement property implementation is not correct, but then I need some clarification
1. What is ImpliedElementProperty ?
2. How do you think InitialValue is applicable at ImpliedElementProperty ?
3. How to retireve the Underlyying model property from ImpliedElementProperty is the getBase() ?
The value property portion is almost correct. You should not be assuming StandardInitialValueService here as it is only one possible implementation. Reference InitialValueService here instead. There should not be an exception in the else clause. An ImpliedElementProperty is similar to ElementProperty in that it holds a child model element. The distinction is that for the implied element property, the child element is always there, so it doesn't need to be created and cannot be deleted. You don't need to do anything explicit to support this property as the same code path will be invoked when child element is instantiated. thanks for the insight, i have made the above metioned corrections, but now when i have a implied propeprty with @InitalValue its not getting seralized the scenario is as follows, i have also enabled the isApplicable in the InitialValue service to make it applicable to ValueProperty and ImpliedElementProperty e.g lets say i have a model with root::child1 root::child2 (implied proeprty) child2::child2-1 child2::child2-2 So when its seralized to xml it looks like, <root> <child1></child1> <child2> <child-2-1></child-2-1> <child-2-2></child-2-2> </child2> </root> So when sapphire lods the model its initial structure will be <root><child2></child2></root> ??? , how can we set the value of Implied proeprty when there is not setter method ? i understand that there is no mutator because it cant be deleted, but then how do we set the value to it to see if the intial value is being set or not ? StandardInitialValueService should not be made applicable to an ImpliedElementProperty. To work with an ImpliedElementProperrty, you call property accessor, which gives you a child model element, on which you can get/set value properties. I am not certain, why nested initial values aren't being set. Maybe you need to access the child element before it will get initialized? Created attachment 201523 [details]
Patch v4
1. Application only for ValueProperty and InitialValue property
2. ModelElementType formatting corrected
3. All uncessary code from ModelElementType removed and the current implementation reduced to just 20 lines
4. test cases adjusted accordingly
Created attachment 201524 [details]
mylyn/context/zip
i guess i got confused with it :( I have now given an implementation which is now setting the initalvalues correctly, "StandardInitialValueService should not be made applicable to an ImpliedElementProperty", yes i found this mistake and corrected. I am attaching the Patch v4, i have ensured that indentation is not distrubed, but still if you feel i need to make correction, please send me the Formatter file you have and i shall use it for any further sapphrie development so that we stay in synch. "I am not certain, why nested initial values aren't being set. Maybe you need to access the child element before it will get initialized?" There could be cases where child elements need to have initial value just like other child element of the root, just to pave for it. If you feel the bug is fixed please changed the status to fixed and i will work on the document/gallery sample part and send it for colosure ModelElementType is still reformatted all over the place. Please start over with a clean copy and carefully apply just the necessary changes. Do not invoke any source formatters. It isn't a matter of you having the same format that I use. Formatters are destructive. Applying my format on my code will change it. It is bad practice in a team environment to be in the habit of invoking code formatter. Doing so introduces unnecessary changes into the source repository and makes it more difficult to see actual changes. Bugs are resolved as fixed when the changes are committed to the repository. Documentation and samples need to be completed before that happens. Created attachment 201535 [details] Patch v5 - ModelElementType updated with HEAD revision and then added the fix for Bug354276 - 0.4/enhancements.html updated for InitialValueService Created attachment 201536 [details]
mylyn/context/zip
1. ModelElementType.instantiate still makes a reference to StandardInitialValueService. 2. InitialValueService now acquired aspects of StandardInitialValueService implementation, breaking the API concept. InitialValueService should be an abstract class with an abstract method "text". It doesn't make any sense to have a string-argument constructor and initialValue member. The whole point of having a custom implementation of InitialValueService is that when called, you will compute the initial value. API cannot assume that the value is known and static at the start. The isApplicable method doesn't belong in this class either. 3. The narative in the enhancements doc is a good start, but is quite confusing, weaving in quite a number of unrelated concepts and using "default value" and "initial value" interchangeably even as we are trying to define those two as separate concepts. There is no need to bring in serialization or XML binding into the narrative. The narrative needs to (a) define what an initial value is (how is it distinct from the default value) and (b) show a simple example of using it. (In reply to comment #53) > 1. ModelElementType.instantiate still makes a reference to > StandardInitialValueService. > Yeah i missed to notice that, now I had made the correction now in Patch v6 > 2. InitialValueService now acquired aspects of StandardInitialValueService > implementation, breaking the API concept. InitialValueService should be an > abstract class with an abstract method "text". It doesn't make any sense to have > a string-argument constructor and initialValue member. The whole point of having > a custom implementation of InitialValueService is that when called, you will > compute the initial value. API cannot assume that the value is known and static > at the start. The isApplicable method doesn't belong in this class either. > I was thinking about it and its usage, now made it abstact. I am going by the Standard Java Bean Accessors/Mutators pattern where the abstract InitialValueService defines two methods setText and getText which are open to subclasses to impelement. This gives us two advantaages, 1. Stick your API design and pattern, allowing us to make addtional computations if need in setText method 2. Making it adaptable to Dependeny Injection pattern in case we introduce it in future Consider all these i removed the Argumented constructor > 3. The narative in the enhancements doc is a good start, but is quite confusing, > weaving in quite a number of unrelated concepts and using "default value" and > "initial value" interchangeably even as we are trying to define those two as > separate concepts. There is no need to bring in serialization or XML binding > into the narrative. The narrative needs to (a) define what an initial value is > (how is it distinct from the default value) and (b) show a simple example of > using it. Updated it with your comments and try to make it as expalanatory and simple as possible. Let me know if you still want some more changes. Created attachment 201538 [details] Patch v6 Refer comment#54 Created attachment 201539 [details]
mylyn/context/zip
> I was thinking about it and its usage, now made it abstact. I am going by the
> Standard Java Bean Accessors/Mutators pattern where the abstract
> InitialValueService defines two methods setText and getText which are open to
> subclasses to impelement. This gives us two advantaages, 1. Stick your API
> design and pattern, allowing us to make addtional computations if need in
> setText method 2. Making it adaptable to Dependeny Injection pattern in case
> we introduce it in future
It does not make sense to apply Java Bean concepts here. It's a service, not an entity. Your ask it to do something and it hands you back the result. In no situation would it make sense to retrieve InitialValueService and do setText on it.
Also note that there is an unnecessary override of the init method in InitialValueService.
It looks like Patch v6 is missing doc changes.
(In reply to comment #57) > > I was thinking about it and its usage, now made it abstact. I am going by the > > Standard Java Bean Accessors/Mutators pattern where the abstract > > InitialValueService defines two methods setText and getText which are open to > > subclasses to impelement. This gives us two advantaages, 1. Stick your API > > design and pattern, allowing us to make addtional computations if need in > > setText method 2. Making it adaptable to Dependeny Injection pattern in case > > we introduce it in future > > It does not make sense to apply Java Bean concepts here. It's a service, not an > entity. Your ask it to do something and it hands you back the result. In no > situation would it make sense to retrieve InitialValueService and do setText on > it. > > Also note that there is an unnecessary override of the init method in > InitialValueService. > Updated, I was not sure that there will not be a setText ... no worries I shall knock it off. > It looks like Patch v6 is missing doc changes. i dont know why but i see my document changes. All these are part of Patch v7 Created attachment 201542 [details] Patch v7 Patch v7 in response to comment#58 Created attachment 201543 [details]
mylyn/context/zip
The docs content is still missing from the patch. Make sure that you select all projects that are part of the change before performing export patch. To make the bug easier to read, please mark the previous version of the patch and mylyn context zip as absolete when attaching a revised copy. (In reply to comment #61) > The docs content is still missing from the patch. Make sure that you select all > projects that are part of the change before performing export patch. I always select all the dirty objects before I create a patch, docs html file is not added to the patch :( ?? Anyways am attaching the Patch v8 I doubled checked the contents . I was really wierd that though the changes were there and CVS showing it as dirty its not getting added to patch. I now took an updated and then inserted the InitialValueService documentation. now I see its added as part of the patch(In reply to comment #62) > To make the bug easier to read, please mark the previous version of the patch > and mylyn context zip as absolete when attaching a revised copy. yes i will do, i was not sure what obselete does so was bit skeptical in using it, now i see it usage i will do it when I see them as obselete. Please check the Patch v8 and hope to have all the content Created attachment 201601 [details]
Patch v8
Patch V8
Created attachment 201602 [details]
mylyn/context/zip
Created attachment 201666 [details]
Patch v9
In the interest of expediency, I took Patch v8 and completed this work item. This included completing the documentation, writing a sample, implementing a FactsService for initial value along with "restore initial value" action, and various cleanup and bug fixes.
Attaching Patch v9 for reference.
Committed patch v9 into 0.4 release stream. Please verify. (In reply to comment #68) > Please verify. Will do. One honest feedback from me, please be bit polite in the way you speak back, as a community memeber am not the one reporting to you or working for your etc., sometimes this could also be a reason why community mebers drop off from middle of their contribution. I understand you lead the framework development and need to provide feedback/review comments on the contributions, but then that could be said a politer way. Sorry if my words are on the harders side. ~Kamesh all other classes looks ok but what happened to the Tests ? Have removed it our moved it to another package ? And thanks for making the classes bit neater and cleaner. I certainly did not mean to offend and I do apologize if my comments were taken as such. On the other hand, it is a common OSS fallacy to assume that every contribution is a net positive. The reality is that every contribution requires time from a committer to get it into acceptable form for merging into the codebase. If X is the time it would take the committer to implement a feature in question and Y is the committer time spent working with a contributor on refining a patch, then you hope that Y << X. That almost never happens when the contributor is new to the project, so you hope that even if first couple of contributions are Y > X, that eventually it will flip. Unfortunately, for this contribution, Y >>> X already, so I thought it would be best to wrap this up myself before the equation got too much more unbalanced. Size of Y in this case can be partly attributed to your inexperience with the contribution process, which is understandable, but I am afraid that a good portion of it was due to lack of attention to detail on your part. Many of the revision cycles would not have been necessary if you would have taken the time to review the exported patch file before contributing it. You would have seen the formatting problems, missing doc changes, etc. without me needing to point them out. I am afraid that after a number of such iterations, my comments likely did get less polite than desired. In any case, I hope you do continue contributing to Sapphire and I hope that in future contributions the equation will start balancing a bit better. ===== Thanks for verifying. The test case is still there. I just renamed it to xml.binding.t0010 as you skipped 10 and went straight to 11. (In reply to comment #71) > I certainly did not mean to offend and I do apologize if my comments were taken > as such. > Thats OK I just wanted to provide my honest fedback to make our team working much smoother and better > On the other hand, it is a common OSS fallacy to assume that every contribution > is a net positive. The reality is that every contribution requires time from a > committer to get it into acceptable form for merging into the codebase. If X is > the time it would take the committer to implement a feature in question and Y is > the committer time spent working with a contributor on refining a patch, then > you hope that Y << X. That almost never happens when the contributor is new to > the project, so you hope that even if first couple of contributions are Y > X, > that eventually it will flip. > > Unfortunately, for this contribution, Y >>> X already, so I thought it would be > best to wrap this up myself before the equation got too much more unbalanced. > Size of Y in this case can be partly attributed to your inexperience with the > contribution process, which is understandable, but I am afraid that a good > portion of it was due to lack of attention to detail on your part. Many of the > revision cycles would not have been necessary if you would have taken the time > to review the exported patch file before contributing it. You would have seen > the formatting problems, missing doc changes, etc. without me needing to point > them out. I am afraid that after a number of such iterations, my comments likely > did get less polite than desired. > Yeah I understand, each Open Source project has its own way of contribution process and the process is bit hectic with Eclipse and I need to make two three contributions to make me accustomed to it and I know/understand how its done now and will avoid few during next contribution. Another point to mention is that the code base is completely new to me including the principles and patterns you follow, i can understand each and everything right away on the first bit of it - that every reviewer needs to understand, its a matter of time to get in to the groove to make contributions on part with the design principles of the framework and also much time needs to be spent to understand and implement properly. Since yo had plans to push to 0.4 version of it i just want to get it our right away.. > In any case, I hope you do continue contributing to Sapphire and I hope that in > future contributions the equation will start balancing a bit better. > I will atleast to clear some of your misconceptions about my contribution and its process .. as and when i try to understand the framework better the more Quality increases, alteast my 11 years of industry experience comes in to play. > ===== > > Thanks for verifying. The test case is still there. I just renamed it to > xml.binding.t0010 as you skipped 10 and went straight to 11. And last comment i personally feel that you as a reviewer create a better Formatter Style for this project, which I personally feel will alleviate few problems realted to formatting, also for every contribution of the project i feel its better the contributor provides a set of files and classes that are going to be modified for this fix earlier on which will pave way for discussions and touching the right classes rather than multiple iterations. More that an WIKI article on the contribution process for sapphire will be much better where you can highlight the steps/care to be taken while dealing with contribution. It looks like you accidentally changed the status with your last comment. Fixing that. I will send you my format style privately. I will not post it as I do not wish to create an impression that everyone must use the same format, which is not the policy of this project. It is the policy of this and most other projects that when a patch is submitted, it should be the minimal set of necessary changes. Re-formatting of surrounding code is frowned upon. The only way not to run into this issue is to not invoke the code formatter when preparing a patch. Using the same format would not help as the formatting is a lossy process. There would still be unnecessary changes. The formatters (regardless of style) also have a habit of mangling structured comments (such as the headers). Hope this helps. |