Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Separate common.datatransfer package into sub-packages #6355 #6453

Merged
merged 2 commits into from
Feb 9, 2017
Merged

Separate common.datatransfer package into sub-packages #6355 #6453

merged 2 commits into from
Feb 9, 2017

Conversation

samsontmr
Copy link
Contributor

Fixes #6355

Problem
teammates.common.datatransfer is presently too cluttered.

Solution
Move files ending with Attributes into .attributes sub package, files related to Questions, Responses into .questions sub package, remaining into .misc sub package.

@samsontmr samsontmr mentioned this pull request Jan 25, 2017
Closed
@wkurniawan07 wkurniawan07 self-assigned this Jan 25, 2017
@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch 4 times, most recently from dc9cab4 to 9856630 Compare January 26, 2017 03:52
@junhaoyap junhaoyap added the s.ToReview The PR is waiting for review(s) label Jan 26, 2017
@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch from e72c19f to 6c15188 Compare January 27, 2017 04:37
@samsontmr
Copy link
Contributor Author

Ready for review @wkurniawan07

@samsontmr
Copy link
Contributor Author

Could someone review this first before I resolve the merge conflicts? I keep resolving and it keeps going out of sync :(

wkurniawan07
wkurniawan07 requested changes Jan 30, 2017
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two preliminary changes requested. I apologize if this requires you to undo many of your hard work!

src/client/java/teammates/client/scripts/AddSectionsToLargeCourses.java Outdated
import teammates.common.datatransfer.CourseDetailsBundle;
import teammates.common.datatransfer.StudentAttributes;
import teammates.common.datatransfer.TeamDetailsBundle;
import teammates.common.datatransfer.attributes.CourseDetailsBundle;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Bundle should not be under attributes sub-package. This sub-package should be strictly for *Attributes.

src/client/java/teammates/client/scripts/DataMigrationForComments.java Outdated
import teammates.common.datatransfer.CommentStatus;
import teammates.common.datatransfer.InstructorAttributes;
import teammates.common.datatransfer.attributes.InstructorAttributes;
import teammates.common.datatransfer.misc.CommentParticipantType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the category misc should not be used, and anything not falling under questions and attributes should be in the datatransfer package instead. Something like:

teammates.common.datatransfer: uncategorised/others
                       .attributes: *Attributes
                       .questions: *QuestionDetails, *ResponseDetails

If anything, this minimizes the changeset of this PR, and there is no significant loss (if any) in organization. Do you have any opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's alright too, i put the *bundle files into .attributes because they seem to be importing mostly attribute files

@wkurniawan07 wkurniawan07 added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jan 30, 2017
@wkurniawan07
Copy link
Member

@samsontmr you need to update with the latest master again. Some may be due to the restructurings done recently (#6431 in particular) because GitHub is not smart enough to recognize file renames, but some may be actual merge conflicts.

@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch 2 times, most recently from 475b653 to 17a2542 Compare January 31, 2017 02:03
@samsontmr
Copy link
Contributor Author

Omg the merge appears to have messed up my changes again

@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch 3 times, most recently from 4f48ae4 to f9a2060 Compare January 31, 2017 05:01
wkurniawan07
wkurniawan07 requested changes Jan 31, 2017
View reviewed changes
src/main/java/teammates/common/datatransfer/FeedbackQuestionType.java Outdated
import teammates.common.datatransfer.questions.FeedbackRubricQuestionDetails;
import teammates.common.datatransfer.questions.FeedbackRubricResponseDetails;
import teammates.common.datatransfer.questions.FeedbackTextQuestionDetails;
import teammates.common.datatransfer.questions.FeedbackTextResponseDetails;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sign that this class should be under .questions sub-package.

Copy link
Contributor Author

@samsontmr samsontmr Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, should FeedbackQuestionBundle be under attributes, questions or bundles then?

src/main/java/teammates/common/datatransfer/FeedbackQuestionDetails.java Outdated
@@ -4,6 +4,8 @@
import java.util.List;
import java.util.Map;

import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes;
import teammates.common.datatransfer.attributes.FeedbackResponseAttributes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the base class of Feedback*QuestionDetails; should be under .questions?

src/main/java/teammates/common/datatransfer/FeedbackResponseDetails.java Outdated
@@ -1,5 +1,8 @@
package teammates.common.datatransfer;

import teammates.common.datatransfer.attributes.FeedbackQuestionAttributes;
import teammates.common.datatransfer.attributes.FeedbackResponseAttributes;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this is the base class for Feedback*ResponseDetails.

@samsontmr
Copy link
Contributor Author

@wkurniawan07 Ready for review

wkurniawan07
wkurniawan07 requested changes Feb 6, 2017
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samsontmr sorry to make you undo-ing your hard work again, but I don't think we should have bundles sub-package yet as of now because the boundaries between that and the base datatransfer package is not so clear. In comparison, the other two subpackages attributes and questions have clear-cut boundaries.

@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch from f2afee7 to d41746b Compare February 6, 2017 10:07
@samsontmr samsontmr closed this Feb 6, 2017
@samsontmr samsontmr reopened this Feb 6, 2017
wkurniawan07
wkurniawan07 requested changes Feb 7, 2017
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but these classes should not be in .questions sub-package:

  • FeedbackQuestionBundle.java
  • FeedbackSessionQuestionsBundle.java
  • FeedbackSessionResponseStatus.java

Other than that, this looks good.

@samsontmr samsontmr force-pushed the 6355-refactor-datatransfer-package branch from d0e7e95 to 51d17e0 Compare February 7, 2017 14:45
wkurniawan07
wkurniawan07 approved these changes Feb 8, 2017
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for your perseverance 👍

And I apologize for changing my mind so many times 😅

@damithc damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 8, 2017
@wkurniawan07 wkurniawan07 added this to the V5.96 milestone Feb 8, 2017
@wkurniawan07 wkurniawan07 merged commit e931146 into TEAMMATES:master Feb 9, 2017
karthikaacharya pushed a commit to karthikaacharya/teammates that referenced this pull request Feb 23, 2017
@samsontmr @karthikaacharya
[TEAMMATES#6355] Separate common.datatransfer package into sub-packag…
1df7655 
…es (TEAMMATES#6453)
@wkurniawan07 wkurniawan07 mentioned this pull request Jun 25, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damithc damithc damithc approved these changes

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

Assignees

@wkurniawan07 wkurniawan07

Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V5.96
Development

Successfully merging this pull request may close these issues.
4 participants
@samsontmr @wkurniawan07 @damithc @junhaoyap