You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by John Burwell <jb...@basho.com> on 2013/06/28 16:06:47 UTC
PrimaryDataStoreEntityImpl TODOs
All,
While reviewing the solidfire patch, I noticed that a dummy getter/setter implementations with TODO comments in the PrimaryDataStoreImpl in were merged into master. I have opened a defect [1] for this issue, as the code will not work as expected (they are not actually working with the internal state of the object as expected).
Speaking generally, every review I have performed for 4.2.0 has contained TODO comments, as well, tab characters and improper indentation. Please check patches for these issues before submission. For me, patches with TODOs are an immediate -1 because it conveys that the implementation is incomplete. The formatting issues are minor, but easily rectified with a quick code format in an IDE.
Thanks,
-John
[1]: https://issues.apache.org/jira/browse/CLOUDSTACK-3277
Re: PrimaryDataStoreEntityImpl TODOs
Posted by John Burwell <jb...@basho.com>.
Prasanna,
More irksome than forgetting to remove TODO comments after addressing
them are TODOs that actually are TODOs and not cruft. The ticket
referenced below us the later. I am fine with having an IDE generate
them as a reminder to finish something. Contributors need to remember
to address and remove them before submission because I see them take
at face value -- there is more work to be done.
Thanks,
-John
On Jun 28, 2013, at 12:08 PM, Prasanna Santhanam <ts...@apache.org> wrote:
> +1
>
> The TODOs, @author tags and minor typos irks me too. Typos are
> understandable and I fix them when I see them. But it we should be
> keep the code clean of IDE introduced comments like TODOs. Please
> consider configuring your IDEs.
>
> Let's all _care_ for our codebase ... just a little.
>
> Now everyone go clean your classes :)
>
> On Fri, Jun 28, 2013 at 10:06:47AM -0400, John Burwell wrote:
>> All,
>>
>> While reviewing the solidfire patch, I noticed that a dummy
>> getter/setter implementations with TODO comments in the
>> PrimaryDataStoreImpl in were merged into master. I have opened a
>> defect [1] for this issue, as the code will not work as expected
>> (they are not actually working with the internal state of the object
>> as expected).
>>
>> Speaking generally, every review I have performed for 4.2.0 has
>> contained TODO comments, as well, tab characters and improper
>> indentation. Please check patches for these issues before
>> submission. For me, patches with TODOs are an immediate -1 because
>> it conveys that the implementation is incomplete. The formatting
>> issues are minor, but easily rectified with a quick code format in
>> an IDE.
>>
>> Thanks,
>> -John
>>
>> [1]: https://issues.apache.org/jira/browse/CLOUDSTACK-3277
>
> --
> Prasanna.,
>
> ------------------------
> Powered by BigRock.com
>
Re: PrimaryDataStoreEntityImpl TODOs
Posted by Prasanna Santhanam <ts...@apache.org>.
+1
The TODOs, @author tags and minor typos irks me too. Typos are
understandable and I fix them when I see them. But it we should be
keep the code clean of IDE introduced comments like TODOs. Please
consider configuring your IDEs.
Let's all _care_ for our codebase ... just a little.
Now everyone go clean your classes :)
On Fri, Jun 28, 2013 at 10:06:47AM -0400, John Burwell wrote:
> All,
>
> While reviewing the solidfire patch, I noticed that a dummy
> getter/setter implementations with TODO comments in the
> PrimaryDataStoreImpl in were merged into master. I have opened a
> defect [1] for this issue, as the code will not work as expected
> (they are not actually working with the internal state of the object
> as expected).
>
> Speaking generally, every review I have performed for 4.2.0 has
> contained TODO comments, as well, tab characters and improper
> indentation. Please check patches for these issues before
> submission. For me, patches with TODOs are an immediate -1 because
> it conveys that the implementation is incomplete. The formatting
> issues are minor, but easily rectified with a quick code format in
> an IDE.
>
> Thanks,
> -John
>
> [1]: https://issues.apache.org/jira/browse/CLOUDSTACK-3277
--
Prasanna.,
------------------------
Powered by BigRock.com