Summary: | [api] TaskMapping should be able to handle custom attributes | ||
---|---|---|---|
Product: | z_Archived | Reporter: | Benjamin Muskalla <b.muskalla> |
Component: | Mylyn | Assignee: | Benjamin Muskalla <b.muskalla> |
Status: | RESOLVED FIXED | QA Contact: | |
Severity: | enhancement | ||
Priority: | P3 | CC: | eclipse, lucas.panjer, ryan.golbeck |
Version: | unspecified | Keywords: | contributed, noteworthy, plan |
Target Milestone: | 3.10 | ||
Hardware: | All | ||
OS: | All | ||
Whiteboard: |
Description
Benjamin Muskalla
2012-08-07 12:44:32 EDT
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! |