Community
Participate
Working Groups
Add some features to the existing Reviews model to cover currently unsupported Gerrit functionality. These changes should still be relatively Review tool neutral, that is, generic enough to be useful for a typical review methodology/system. See https://git.eclipse.org/r/#/c/10909/ #Made references between ReviewItem review and Review items opposites. This is neccesary to support properly accessing these values, but might cause issues for R4E. Sebastien, let's test and discuss. #Added Committed By and Reference attributes to ReviewItem Note that we'll need to regenerate R4E code to support. See https://git.eclipse.org/r/#/c/10547/
Miles, are you planning to make further changes to the model?
Yep, there will be some for the current experimental review that I'm about to break up. I'd like to hold this one open until we finish the next round.
In fact, there are some more extensive changes that we need to make a judgment on soon. There are a number of approvals related constructs that need to be covered for http://git.eclipse.org/c/mylyn/org.eclipse.mylyn.reviews.git/tree/org.eclipse.mylyn.gerrit.ui/src/org/eclipse/mylyn/internal/gerrit/ui/editor/ReviewSection.java The question is whether to: a) try to come up with reasonable generalizations for the current Reviews model, at the risk of pushing Gerrit semantics into reviews. b) create a gerrit specific model implementation, at minor dev effort, but significant maintenance commitment, or c) keep the existing implementation, which would mean that we wouldn't have good support for remote users, let alone offline updates. Thoughts?
(In reply to comment #3) > a) try to come up with reasonable generalizations for the current Reviews > model, at the risk of pushing Gerrit semantics into reviews. Can you provide a list of concepts that are Gerrit specific and don't fit into the current model? > b) create a gerrit specific model implementation, at minor dev effort, but > significant maintenance commitment, or -1 we should aim to avoid that. I'd rather create a model that has some flexibility built in and optional parts than maintaining a separate model for Gerrit. > c) keep the existing implementation, which would mean that we wouldn't have > good support for remote users, let alone offline updates. Not sure what the implications of this are. I consider offline functionality of the Gerrit connector broken at the moment so the status quo is not a viable option in my opinion. We would then have to discuss rolling back the remote API changes which I don't think anybody wants.
(In reply to comment #4) > > b) create a gerrit specific model implementation, at minor dev effort, but > > significant maintenance commitment, or > > -1 we should aim to avoid that. I'd rather create a model that has some > flexibility built in and optional parts than maintaining a separate model for > Gerrit. I agree that the last thing I want to do is implement that now. OTOH, we will have to support additional review flavors/types for R4E and presumably others, so it wouldn't be an unreasonable option if we had plenty of time, which we don't. :) I think it is likely we'll want to return to revisit this later though. > > c) keep the existing implementation, which would mean that we wouldn't have > > good support for remote users, let alone offline updates. > > Not sure what the implications of this are. I consider offline functionality of > the Gerrit connector broken at the moment so the status quo is not a viable > option in my opinion. We would then have to discuss rolling back the remote API > changes which I don't think anybody wants. Agreed. c) isn't a real option, but I included it for thoroughness. One thing that I didn't appreciate at the beginning of this exercise was the neat trick the Gerrit remote connector uses to persist offline and the implications of that for our attempt to change the airplane engine while we're in the air. The connector gathers everything into the Gerrit object using the Gerrit APIs, and then freeze-dries it via JSON to task data. So basically the connector can treat many of the Gerrit constructs as if they are live. However, there actually *never were* model analogs for much of the key Gerrit functionality. So we're not really re-implementing or refactoring this functionality, we're implementing it for the first time. But as I said earlier, there is *no half way done here* -- we're committed to putting these into the model or we won't have any way to persist them. Since we need to deal w/ this asap, that implies a) : making the changes in the reviews model, perhaps with a view toward generalizing them further or off-loading them later. What we need is basically what builds up the Review Peoples matrix, requirements*, and the dependencies. I don't think we need to surface a lot of the stuff that's in the Gerrit model, so probably something like the below: ___People___ UserApproval User user ApprovalItem* approvals ApprovalItem ApprovalType type Integer value ApprovalType String id; (e.g. "Verified", "Code Review", Reviews UserApproval approvals ReviewGroup ApprovalType* approvalsTypes (I'm not actually happy w/ "Approval" as a name, but I can't think of a better one offhand.) ___Requirments___ I'm actually not sure how the requirements is supposed to function; I can't find a review that actually has that section, e.g. that has items for "missing approvals". Does anyone have an example of this? ___Dependencies___ Change (New) id subject message state Review -> Change... Change* parents Change* children
From our point of view, a) is by far the preferred solution. It does not matter much to us if some the the constructs are a little gerrit-specific, as R4E will be aligned more and more with Gerrit anyways. In any case we will extend the base model and can just not use some of the constructs if we do not need them. Of course optionality is a must for those. b is not really acceptable, nor is c, as one of the main new features we want to introduce is the ability to review code locally (offline). Also the remotes API is of particular interest for us, since we will implement our own remote persistence mechanism with it, as well as using it to persist the data on a gerrit server as an option to our users.
Thanks fore the detailed list! Sounds like we are all in agreement that a) is the way to go. From my point of view it looks like we can represent the missing Gerrit concepts in the model. In terms of extensibility I was thinking that some objects could have key/value maps or a similar mechanism that support storing additional data. With JSon it's very simple to store complex types as well if there are internal Gerrit IDs or the like need to be persisted but don't have meaning in the reviews model otherwise. If we don't need it great but it's something to explore before we extend the model. (In reply to comment #5) > ___People___ > > UserApproval > User user > ApprovalItem* approvals > > ApprovalItem > ApprovalType type > Integer value > > ApprovalType > String id; (e.g. "Verified", "Code Review", > > Reviews > UserApproval approvals > > ReviewGroup > ApprovalType* approvalsTypes This all sounds reasonably generic and we can come up with different terminology if the names don't fit well. > ___Requirments___ > > I'm actually not sure how the requirements is supposed to function; I can't find > a review that actually has that section, e.g. that has items for "missing > approvals". Does anyone have an example of this? These are the categories that are missing votes. I don't see them in the review editor so it looks like this broke. In the web UI they are listed under the reviewers table, e.g. on https://git.eclipse.org/r/#/c/2609/: * Need Code-Review * Need IP-Clean I'll file a separate bug for the missing requirements. In terms of the model it seems reasonable to represent requirements. > ___Dependencies___ > > Change (New) > id > subject > message > state > > Review -> Change... > Change* parents > Change* children This sounds very generic as well and would make sense to represent in the model.
Important note: In Gerrit, approvals are actually based on patch sets, not the review as a whole. Change Detail does allow retrieval of approvals for change detail, and that appears to be what the current editor is using for the Reviews detail section. So to be clear, we will not be supporting these at the patch set / ReviewItemSet level, just for the review as a whole.
___Dependencies___ Another issue -- one that we don't need to resolve right away -- is what to do in the case where a change matches some other review change, or when multiple reviews have dependencies on the same change. Currently, I'm just planning to have a simple containment on Reviews and allow these to be duplicated, but at some point it might make sense to unify these into a common graph so that we would be able to trace all change dependencies across reviews.
Sebastien, I'd like to get rid of ReviewGroup#ReviewGroupTask. Do you have a problem with that?
Sebastien, also note that I'm creating: Repository <- ReviewGroup -approvalTypes -reviewStates Now, I'd like to change the type/name of Review#group to Review#repository, but I'm not sure that that will work for R4E. Is this really a seperate concept? If so, can we accomodate it seperatly somehow?
Miles, For the ReviewGroup#ReviewGroupTask, you can get rid of it, as we do not use it in R4E. The new elements should be okay, as long as they are optional. For changing Review#group to Review#repository, yes that would break the R4E persistency. However if needed we can use our upgrade engine to correct the serialized data when upgrading to the new version, but this is not the preferred solution.
(In reply to comment #12) > For changing Review#group to Review#repository, yes that would break the R4E > persistency. However if needed we can use our upgrade engine to correct the > serialized data when upgrading to the new version, but this is not the preferred > solution. I think we can keep it as a group reference. That also allows us more flexibility in allowing reviews to be organized. We'll either cast it to repository where neccessary, or I may add a derived convenience method. Sebastien, re: ReviewComponent is also something I'd like to get rid of, since now it only has this enabled flag. We don't have to do this now, but I can't remember if this was neccessary and might be replaced by some other capability in R4E? Like, you might not need to do the enabled stuff if you aren't worried about file contention issues with remote API.
Sebastien, one other issue. I've removed Review#items and replaced it with Review#sets, as at least to my understanding, reviews should never contain items directly -- they should always be part of a review set. But I'm thinking that R4E may be making a different assumption. See RModelFactoryExtImpl L 612. Seems to be expecting an arbitrary item? 1) Does this change make sense to you, and 2) If so, will this change cause other difficulties? It would require a model transformation, but I'm beginning to think that's inevitable to have a really clean Review model, and we should avoid having the R4E tail wagging the Reviews dog, as it were. ;D
(In reply to comment #13) > (In reply to comment #12) > > For changing Review#group to Review#repository, yes that would break the R4E > > persistency. However if needed we can use our upgrade engine to correct the > > serialized data when upgrading to the new version, but this is not the preferred > > solution. > > I think we can keep it as a group reference. That also allows us more > flexibility in allowing reviews to be organized. We'll either cast it to > repository where neccessary, or I may add a derived convenience method. > > Sebastien, re: ReviewComponent is also something I'd like to get rid of, > since now it only has this enabled flag. We don't have to do this now, but I > can't remember if this was neccessary and might be replaced by some other > capability in R4E? Like, you might not need to do the enabled stuff if you > aren't worried about file contention issues with remote API. The Review Component enabled flag is used in R4E to restrict visibility i.e. when the derived element is obsoleted/deleted by the user then we really set its enabled state to false and we filter it out. I could actually see this being useful in the common base e.g. to filter out abandoned gerrit reviews etc. I is just an abstraction of any model element and could stay there fow now I think. Otherwise we would have to alter the derived R4E model
(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #12) > > > For changing Review#group to Review#repository, yes that would break the R4E > > > persistency. However if needed we can use our upgrade engine to correct the > > > serialized data when upgrading to the new version, but this is not the preferred > > > solution. > > > > I think we can keep it as a group reference. That also allows us more > > flexibility in allowing reviews to be organized. We'll either cast it to > > repository where neccessary, or I may add a derived convenience method. > > > > Sebastien, re: ReviewComponent is also something I'd like to get rid of, > > since now it only has this enabled flag. We don't have to do this now, but I > > can't remember if this was neccessary and might be replaced by some other > > capability in R4E? Like, you might not need to do the enabled stuff if you > > aren't worried about file contention issues with remote API. > > The Review Component enabled flag is used in R4E to restrict visibility i.e. > when the derived element is obsoleted/deleted by the user then we really set > its enabled state to false and we filter it out. I could actually see this > being useful in the common base e.g. to filter out abandoned gerrit reviews > etc. I is just an abstraction of any model element and could stay there fow > now I think. Otherwise we would have to alter the derived R4E model (In reply to comment #14) > Sebastien, one other issue. I've removed Review#items and replaced it with > Review#sets, as at least to my understanding, reviews should never contain > items directly -- they should always be part of a review set. But I'm > thinking that R4E may be making a different assumption. See > RModelFactoryExtImpl L 612. Seems to be expecting an arbitrary item? > > 1) Does this change make sense to you, and 2) If so, will this change cause > other difficulties? It would require a model transformation, but I'm > beginning to think that's inevitable to have a really clean Review model, > and we should avoid having the R4E tail wagging the Reviews dog, as it were. > ;D Yeah there we arrive at the main difference between the original common model and the version Steffen forked at the time. In the original vision, the item element was supposed to mean an item of the review i.e. a patch set. However in steffen's version he uses Review Item for a completely different purpose. So yes having this change will cause backward compatibility problems in R4E and we will need to have some kind of transformation. OTOH the Kepler version is already shaping to be a big change anyways, so we might as well cram every model change we want and maybe we'll just say that the R4E version based on it is not backward compatible. By the same token I think your effort is the best time to change to model with the changes in bug 371428 comment 6 and bug 403393 i.e. changing Mylyn Reviews IFileRevision to IFileVersion and introduce a reference to the team API repository at the review level and a reference to the team API IFileRevision in the Mylyn IFileVersion.
(In reply to comment #15) > The Review Component enabled flag is used in R4E to restrict visibility i.e. > when the derived element is obsoleted/deleted by the user then we really set its > enabled state to false and we filter it out. I could actually see this being > useful in the common base e.g. to filter out abandoned gerrit reviews etc. I is > just an abstraction of any model element and could stay there fow now I think. > Otherwise we would have to alter the derived R4E model It isn't really harming anything though it does make things a bit more complex then they'd otherwise need to be. My chief issue is actually that it's really a UI concern and I think user specific. When we revisit, this we should consider having a separate UI/configuration model that the API could query to discover stuff like this. This could be part of a model that supports say setting local priority levels, time spent, etc.. -- stuff that is applicable to lot's of model objects and is currently stored in task data. (In reply to comment #16) > Yeah there we arrive at the main difference between the original common model > and the version Steffen forked at the time. In the original vision, the item > element was supposed to mean an item of the review i.e. a patch set. However in > steffen's version he uses Review Item for a completely different purpose. So > yes having this change will cause backward compatibility problems in R4E and we > will need to have some kind of transformation. OTOH the Kepler version is > already shaping to be a big change anyways, so we might as well cram every model > change we want and maybe we'll just say that the R4E version based on it is not > backward compatible. I agree. And we do have the regexp search and replace mechanism I built last year to hopefully take care of stuff like this. But I'm not sure if you're saying that this will also cause difficulties for you because it doesn't match up w/ the actual R4E design (vs. strcuture)? > By the same token I think your effort is the best time to change to model with > the changes in bug 371428 comment 6 and bug 403393 i.e. changing Mylyn Reviews > IFileRevision to IFileVersion and introduce a reference to the team API > repository at the review level and a reference to the team API IFileRevision in > the Mylyn IFileVersion. Yeah, I wondered about doing that now too, it seemed like a seperate concern, but I agree that we should just go ageand and do that now. I'll update the review with this change.
Please review: https://git.eclipse.org/r/#/c/11795/ (In reply to comment #17) > > the changes in bug 371428 comment 6 and bug 403393 i.e. changing Mylyn Reviews > > IFileRevision to IFileVersion and introduce a reference to the team API > > repository at the review level and a reference to the team API IFileRevision > in > > the Mylyn IFileVersion. > > Yeah, I wondered about doing that now too, it seemed like a seperate concern, > but I agree that we should just go ageand and do that now. I'll update the > review with this change. Changed my mind. :) I think since we have a seperate bug already and this one is already very crowded, we should handle this as a seperate review.
Copying Review description here: •Gerrit Related: Added Review parents, children, reviewerApprovals supported by RequirementEntry, RequirementStatus, ReviewRequirementsMap and RequirementReviewState entities New Change class pushes id, key, subject, message, owner and state down from Review •Persistence Support: Added containment for TopicContainer#directTopics Removed derived from ReviewGroup#reviews, Review#review Created containment reference for ReviewGroup#reviews, ReviewGroup#users, ReviewItem#items, Topic#comments and opposites •Other: Removed obsolete #reviewTask from Review Set ReviewComponent#enabled id property to false Added descriptor to ReviewState to support generic naming and SimpleReviewState for extension Changed Review#items -> Review#sets and type from ReviewItem -> ReviewItemSet
For R4E, please review https://git.eclipse.org/r/#/c/11834/ *Note that this is now broken!* See my comments. That's because of the change from Review#items to Review#sets. Note that it is not a matter of simply renaming, it is a type issue. I need to know whether it is safe to check for IReviewItemSet here and then cast to it, or if it is indeed possible to have a non-IReviewItemSet IReviewItem here. If the latter, we need to revisit https://git.eclipse.org/r/#/c/11795/.
Merged: https://git.eclipse.org/r/#/c/11795/
Resolving. Further model changes will happen under new bugs.
