Community
Participate
Working Groups
Right now, the set of attributes supported by @TaskMapping@ / @ITaskMapping@ is limited to a few. Many connectors need other attributes to initialize their TaskDatas and thus extend TaskMapping to introduce new setters/getters (eg. getProjectArea). While this is ok from the connector POV, it makes it really hard for API consumers to use these connectors. As consumer I need to a) know which class the connector uses as it's specific @TaskMapping@ b) instantiate the class with the right values This means that Mylyn API consumers need to access internal classes of the connector as most connectors check and cast the ITaskMapping to their own class in order to access the additional details. @ITaskMapping@ should rather have a generic setAttribute/getAttribute concept (baked by an @AttributeMap@) to let connectors handle this in a lightweight and streamlined way. With this approach, API consumers don't need to know about the internals of the connector and just need to provide the right set of key/value pairs of the connector. Dynamically exploring which properties are needed is tracked on task 367372.
Sounds reasonable to me to provide a default implementation that is generic enough to capture the data model required for task creation. I believe most connectors simply pass String fields around and don't have a need for implementing custom behavior in the mapping so the proposed solution should work.
Yeah, all connectors I've seen so far would be fine with Strings. Before starting on this one: Given we pass ITaskmapping and TaskMapping around in APIs, I think it would make sense to enhance those too with the methods and not just provide a generic implementation as subclass, I think this should be part of the interface contract. Given ITaskMapping is @noimplement, we would not even break API (at least no one who's playing nice). If we don't do this, it's up to the connectors to properly check and cast onto a GenericTaskMapping
ITaskMapping doesn't have any setters so what would be adding to the interface? public String getAttribute(String key) ? I'm not sure I like that since it's not obvious what implementors would need to return, e.g. if TaskAttribute.PRIORITY is passed.
We'd need to add setAttribute and getAttribute. I think it should depend on the implementation if they handle this separately from all the other getters or if they map the attribute to one of the getters (eg. setAttribute(TaskAttribute.TASK_KIND -> getTaskKind).
I'm -1 on adding the setter to the interface, it would completely change the contract.
Yeah maybe because ITaskMapping is used in two different scenarios. From a contract-sense, I think it's the right approach for the initializeTaskData case. If we don't have it in the interface, this means that connectors are forced to cast it down to TaskMapping (or even a subclass of that if we don't put it into TaskMapping) which then has to be made API.
I see your point and the reuse of ITaskMapping for that task initialization isn't particularly intuitive. I don't see a good way to extend the existing API without making it more complicated though. To still be able to provide a common API for task initialization we discussed the following idea: * Provide a new API class named TaskInitializationData that implements ITaskMapping and provides an interface for a key value map of strings. The class should be marked as @noextend. * The class should implement the getters that return Strings and throw UnsupportedOperationException for all other methods. * Connectors are expected to use that class to manage initialization data. This should be explicit in the JavaDoc. Benny, feel free to bounce to me if you don't have bandwidth to implement this.
Posted a first draft at https://git.eclipse.org/r/8735
Benny, looks like this is almost done. Do you have time to take another pass at the change and rebase it against the latest?
The change was merged. Thanks, Benny! Connectors are now encouraged to use TaskInitializationData implementations to specify the initialization for creating tasks, e.g. in in NewTaskWizard.
This has caused a regression in tests: java.lang.UnsupportedOperationException at org.eclipse.mylyn.tasks.core.TaskInitializationData.getTaskData(TaskInitializationData.java:158) at org.eclipse.mylyn.tasks.core.data.TaskMapper.merge(TaskMapper.java:145) at org.eclipse.mylyn.bugzilla.tests.BugzillaTaskDataHandlerTest.testCloneTaskData(BugzillaTaskDataHandlerTest.java:59)
It looks like Bugzilla repository tests did not run as part of the suite: https://hudson.eclipse.org/sandbox/job/mylyn-tasks-gerrit/678/. Frank, do you have any ideas why that would be the case?
I pushed 2 reviews to fix the test case and fail if no fixtures are discovered which appears to be the case for Bugzilla: https://git.eclipse.org/r/#/c/14325/ https://git.eclipse.org/r/#/c/14324/
(In reply to comment #13) > I pushed 2 reviews to fix the test case and fail if no fixtures are > discovered which appears to be the case for Bugzilla: > > https://git.eclipse.org/r/#/c/14325/ > https://git.eclipse.org/r/#/c/14324/ We get no fixtures if we use Hudson. Maybe this is because of the http proxy. mylyn-tasks-gerrit job #599 (713 tests) we have since job #603 (541 tests)
Frank, I submitted your fix and added proxy initialization code to fix the test fixture discovery. I opened bug 412743 to discuss improving that though. Thanks very much for your help!
