Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1350
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-216423449
@swill yes that's what I meant, therefore commented that it's ready for
merge. Thanks.
---
If your project is set up for it, you can reply to this email and have
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-216308956
@rafaelweingartner thanks for the review. :) I think what @rhtyd meant is
anything outstanding is only cosmetic, I don't think he meant the entire
discussion.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-216307184
@swill,
I did a review on the comments and discussions to get me up to date (we had
a discussion about some code duplications). Those discussions
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-216300372
@rafaelweingartner This one seems to be in OK shape. I am missing one code
review, given that you have actively reviewed this code, can I get your final
status?
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-216221000
LGTM, there are some comments on cosmetics but in general we should get
this CI tested and merged cc @swill
tag:easypr
---
If your project is set up for
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-200186297
### ACS CI BVT Run
**Sumarry:**
Build Number 121
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=14
Skipped=4
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-176082402
@DaanHoogland I agree there should be some guideline on code style. If
these are there I think most of us will be willing to follow these. I guess it
will not
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-176080946
@agneya2001 Sounds like you want to set the style guide. you and
@rafaelweingartner should take this discussion to a higher level and take it to
a discuss
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175931632
This code that was suggested:
private AccountVO createAccountVoForType(short type) {
AccountVO accountVO = new AccountVO();
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175702905
@bhaisaab I think the test cases in different methods, each one for a
single and unit test is the right way to go.
What I disagree is with the
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1350#discussion_r51005837
--- Diff:
framework/quota/test/org/apache/cloudstack/quota/QuotaManagerImplTest.java ---
@@ -197,4 +197,58 @@ public void
Github user agneya2001 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1350#discussion_r51006727
--- Diff:
framework/quota/test/org/apache/cloudstack/quota/QuotaManagerImplTest.java ---
@@ -197,4 +197,58 @@ public void
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175693496
@rafaelweingartner fair points, but it would make sense if you can comment
on the diff in the context of the change.
Specific to the PR I see several
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175708121
@agneya2001 how about you have a look at the test tomorrow and see if you
can incorporate some of the suggestions like pull out the account from the
method as a
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175710627
@rafaelweingartner @bhaisaab I have taken out the duplicate account
initialisation.
---
If your project is set up for it, you can reply to this email and have
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175712977
@cristofolini your changes are incorporated can you review these.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175816413
I disagree that it would make the code more complex.
That is what I meant:
```
private AccountVO createAccountVoForType(short type) {
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175563484
@agneya2001 LGTM again, need one more review to get this merged along with
some testing (though in this case, we've some unit tests for the change)
---
If your
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175656788
@rafaelweingartner It is only about realistic test data that is being
passed to the method tested. You should differentiate between the tested code
and the
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175634392
@rafaelweingartner If it is a black box test then the person testing will
obviously try to pass a realistic data set, that is what I have done. It is
possible
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175637289
If in a remotely distant future someone that we do not even know yet
changes that method and make use of some extra data (forgetting to check the
test
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175599430
Hi @agneya2001, there are a lot of duplicated lines at
âQuotaManagerImplTestâ, would you mind removing them?
Additionally, the lines you use
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175630573
It is cultural; we have to stop duplicating lines; no matter where and no
matter how simple those blocks may seem.
We also have to stop coding
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175609511
@agneya2001 that is exactly what I meant.
I disagree with you, letting this as they are, it is not simple. Why
duplicate code, when we can do
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175624327
@rafaelweingartner quality is what we are discussing and I simply do not
think that putting 3 lines of object initialisation in a method will make it
high
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175601093
@rafaelweingartner Can you comment on the duplicated lines. There is a way
you can get to the code and comment there.
---
If your project is set up for it,
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175601814
I know that. I just did not want to go one by one and tell you that they
are duplicated, I believe you can easily see that. Those duplicated lines I
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175603740
@rafaelweingartner if you mean initialisation of AccountVO to be moved into
a method, and replacing it with a method call. I guess I would prefer it this
way as
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175667113
I do not agree.
If people are comfortable with it, so let it be.
---
If your project is set up for it, you can reply to this email and have your
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175408926
@cristofolini @bhaisaab @remibergsma test code split into simpler methods.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-174211441
I see you've put a bunch of different test cases into a single
`testLockableAccount` method. I'd strongly suggest you put each of those
different cases into
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-173218775
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
33 matches
Mail list logo
