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.

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

LPS-55718 #417

Closed
wants to merge 42 commits into from
Closed

LPS-55718 #417

wants to merge 42 commits into from

Conversation

drewbrokke
Copy link

rotty3000 and others added 30 commits June 8, 2015 12:13
@ealonso
Copy link
Owner

ealonso commented Jun 12, 2015

Just started reviewing :)

:octocat: Sent from GH.

@ealonso
Copy link
Owner

ealonso commented Jun 12, 2015

Hi @drewbrokke,

I've sent a few commits to Brian: see brianchandotcom#27296

A few things about the pull:

  1. About the last commit "ant format-source": it is better to merge it with each file, because if you put it at the end it is more difficult to review the pull commit by commit.
  2. When we make a commit like "Makes methods public, no logic changes" we should do an "ant format-source". This is the purpose for this commit, so us to make easier to understand the next commit.
  3. Regarding ActionCommad, we should use it only in the necessary cases, for example, if you have an Action in a separate file you can create an ActionCommad with this action. In our case, for Delete and Update methods, it is really rare create two different classes and split the methods, IMHO we should put it on the portlet class.

Could you make those changes and resend?

Thanks!!!

@ealonso ealonso closed this Jun 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants