Re: svn commit: r818335 - /ofbiz/trunk/applications/product/webapp/catalog/WEB-INF/actions/product/EditProductContentContent.groovy

[Adam Heath-2]

[hidden email] wrote:
> Author: lektran
> Date: Thu Sep 24 00:56:30 2009
> New Revision: 818335
>
> URL: http://svn.apache.org/viewvc?rev=818335&view=rev
> Log:
> Misc. cleanups

And those cleanups are?

[Scott Gray-2]

Miscellaneous

On 30/09/2009, at 5:51 AM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: lektran
>> Date: Thu Sep 24 00:56:30 2009
>> New Revision: 818335
>>
>> URL: http://svn.apache.org/viewvc?rev=818335&view=rev
>> Log:
>> Misc. cleanups
>
> And those cleanups are?

[Adam Heath-2]

A: Because it changes the flow of a conversion.

Q: Why is top-posting frowned upon?

Scott Gray wrote:
> Miscellaneous

They aren't.

* Access parameters thru the parameters map, instead of on the request
object directly.
* Use groovy empty object logic, instead of comparing values against
an empty string.
* Make use of groovy iteration support, thru each.

What I wrote explains it much better.  If there is some *real* error
that someone is trying to track down, and they see this 'misc changes'
log entry, they have no idea what was actually done, so they have to
run a diff of the changeset.  If they see the above changelog entries,
then they know they can keep going, 'cuz more than likely it(the bug
they are trying to find) is not in this particular changeset.

[Adrian Crum]

A. Frowning upon top-posting.

Q. What causes those deep creases in the forehead?

Adam Heath wrote:

> A: Because it changes the flow of a conversion.
>
> Q: Why is top-posting frowned upon?
>
> Scott Gray wrote:
>> Miscellaneous
>
> They aren't.
>
> * Access parameters thru the parameters map, instead of on the request
> object directly.
> * Use groovy empty object logic, instead of comparing values against
> an empty string.
> * Make use of groovy iteration support, thru each.
>
> What I wrote explains it much better.  If there is some *real* error
> that someone is trying to track down, and they see this 'misc changes'
> log entry, they have no idea what was actually done, so they have to
> run a diff of the changeset.  If they see the above changelog entries,
> then they know they can keep going, 'cuz more than likely it(the bug
> they are trying to find) is not in this particular changeset.
>
>

[Scott Gray-2]

Fair enough point taken, except for the top-posting thing.

Regards
Scott

On 30/09/2009, at 10:02 AM, Adam Heath wrote:

> A: Because it changes the flow of a conversion.
>
> Q: Why is top-posting frowned upon?
>
> Scott Gray wrote:
>> Miscellaneous
>
> They aren't.
>
> * Access parameters thru the parameters map, instead of on the request
> object directly.
> * Use groovy empty object logic, instead of comparing values against
> an empty string.
> * Make use of groovy iteration support, thru each.
>
> What I wrote explains it much better.  If there is some *real* error
> that someone is trying to track down, and they see this 'misc changes'
> log entry, they have no idea what was actually done, so they have to
> run a diff of the changeset.  If they see the above changelog entries,
> then they know they can keep going, 'cuz more than likely it(the bug
> they are trying to find) is not in this particular changeset.
>

[David E. Jones-2]

My opinion, for what it's worth:

Top posting: IDC

Commit comments: Adam's right, most of them are useless and might as  
well just say "changed stuff" all the time; I'm glad someone is  
complaining about this!

-David


On Sep 29, 2009, at 3:22 PM, Adrian Crum wrote:

> A. Frowning upon top-posting.
>
> Q. What causes those deep creases in the forehead?
>
> Adam Heath wrote:
>> A: Because it changes the flow of a conversion.
>> Q: Why is top-posting frowned upon?
>> Scott Gray wrote:
>>> Miscellaneous
>> They aren't.
>> * Access parameters thru the parameters map, instead of on the  
>> request
>> object directly.
>> * Use groovy empty object logic, instead of comparing values against
>> an empty string.
>> * Make use of groovy iteration support, thru each.
>> What I wrote explains it much better.  If there is some *real* error
>> that someone is trying to track down, and they see this 'misc  
>> changes'
>> log entry, they have no idea what was actually done, so they have to
>> run a diff of the changeset.  If they see the above changelog  
>> entries,
>> then they know they can keep going, 'cuz more than likely it(the bug
>> they are trying to find) is not in this particular changeset.

[Adam Heath-2]

David E Jones wrote:
>
> My opinion, for what it's worth:

Opinions: $5
Dirty Looks: Free

> Commit comments: Adam's right, most of them are useless and might as
> well just say "changed stuff" all the time; I'm glad someone is
> complaining about this!

Examples of badness:

--

* Fixed bug (reported by $user)? from $location.

When reading history, the full discussion leading up to a proper bug
fix is not important.  But a description of the bug, and the solution,
are nescessary.

Quite often, when trying to find a bug, the original sources are *not*
available.  All you have at your disposable is the installed system
image.  The changelog describing the software installed very often
accompanies the installation.

It's quite possible that *no* outside access is available.  Embedded
system deployments do this quite frequently.

When scanning a changelog for possible hints into why something
is/isn't working, if I have to switch back and forth from the target
system, and an external system, to figure out what is going on, then
it will leave a sour taste in my mouth.  Going forward, I would be
reluctant to recommend the software again.

(this next bit is a general statement, not directed at any one person)

Additionally, no one is you.  There are 6 billion other people on this
planet, and *you* are the only one who knows what *you* are thinking.
 We can't read minds.  It becomes even more difficult to do so, 2-3(or
more) years in down the road.  So, describe what you have done at the
time you have done it.

Also, not everone knows how jira works.  Or confluence.  Or
AutoConfigMaintenceWidgetApplication.  What you may thing as a
sensible cross-reference(OFBIZ-####, debbugs ####) may mean nothing to
the person reading your changelog.

--

* I did some things in FooClass, FooImpl, some more stuff in
BarWidget, and yet more interesting tidbits in YellowSubmarine;
basically, stuff all over the place.

This bad changelog is not about poor descriptions of the changes.
It's about listing multiple, 100% completely separate
improvements/fixes into a single commit.

If there were 10 lines changed, and 1 line was not related, it's
possible that you could still notice that 1 extra line.  But if you
have 1000 lines changes, and there are 3 groups of changes, and they
are all intermingled, it becomes *very* difficult to very that each
separate change was actually implemented correctly.

It's much better to split up such changes, and commit them separately.
 There are several ways to do this.  svk/hg/git can be used to mirror
svn, have local changes, then push when you are done.  You can use
multiple svn checkouts, and redo your changes, one step at a time.

<rant>
I'm willing to overlook grouped commits some times.  However, I
absolutely, positively, 100% *DETEST* "I can't remember what I did"
type entries.  ARG!  If you don't know how to use the basic features
of the revision control tool, then find some other industry to work
in.  *ALL* revision tools support listing of changes.  *ALL*.
</rant>

--

There is an old axiom, that states that methods(functions,
subroutines, whatever they are called nowdays) should not be longer
than a page.  While this doesn't completely hold nowadays(this came
about during the physical printout era), the underlying principle
still applies.  Smaller is easier to understand.  This goes for line
lengths, methods, files(classes), and even to changesets.

Also, one of the criteria I use in evaluating software, is how easy
*non-associated* people have at fixing the problems that come up.
Software has bugs; this is just the nature of the business.  And the
bugs that tend to come up in production are the ones that occur
*years* after deployment, after you've moved on to other things.

Years after the deploy has occurred, you don't remember the little
details; sometimes, you can't even recall the big ones.  And when
trying to track down the *cause* of a problem, it generally is trying
to find out *when* something broke.  Proper changelogs are the first
step in figure out the when.

[Jacques Le Roux]

Thank you Adam,

I posted your comments (without rants and typos ;o) at
http://docs.ofbiz.org/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities#OFBizCommittersRolesandResponsibilities-CommittingChanges

Jacques

From: "Adam Heath" <[hidden email]>

> David E Jones wrote:
>>
>> My opinion, for what it's worth:
>
> Opinions: $5
> Dirty Looks: Free
>
>> Commit comments: Adam's right, most of them are useless and might as
>> well just say "changed stuff" all the time; I'm glad someone is
>> complaining about this!
>
> Examples of badness:
>
> --
>
> * Fixed bug (reported by $user)? from $location.
>
> When reading history, the full discussion leading up to a proper bug
> fix is not important.  But a description of the bug, and the solution,
> are nescessary.
>
> Quite often, when trying to find a bug, the original sources are *not*
> available.  All you have at your disposable is the installed system
> image.  The changelog describing the software installed very often
> accompanies the installation.
>
> It's quite possible that *no* outside access is available.  Embedded
> system deployments do this quite frequently.
>
> When scanning a changelog for possible hints into why something
> is/isn't working, if I have to switch back and forth from the target
> system, and an external system, to figure out what is going on, then
> it will leave a sour taste in my mouth.  Going forward, I would be
> reluctant to recommend the software again.
>
> (this next bit is a general statement, not directed at any one person)
>
> Additionally, no one is you.  There are 6 billion other people on this
> planet, and *you* are the only one who knows what *you* are thinking.
> We can't read minds.  It becomes even more difficult to do so, 2-3(or
> more) years in down the road.  So, describe what you have done at the
> time you have done it.
>
> Also, not everone knows how jira works.  Or confluence.  Or
> AutoConfigMaintenceWidgetApplication.  What you may thing as a
> sensible cross-reference(OFBIZ-####, debbugs ####) may mean nothing to
> the person reading your changelog.
>
> --
>
> * I did some things in FooClass, FooImpl, some more stuff in
> BarWidget, and yet more interesting tidbits in YellowSubmarine;
> basically, stuff all over the place.
>
> This bad changelog is not about poor descriptions of the changes.
> It's about listing multiple, 100% completely separate
> improvements/fixes into a single commit.
>
> If there were 10 lines changed, and 1 line was not related, it's
> possible that you could still notice that 1 extra line.  But if you
> have 1000 lines changes, and there are 3 groups of changes, and they
> are all intermingled, it becomes *very* difficult to very that each
> separate change was actually implemented correctly.
>
> It's much better to split up such changes, and commit them separately.
> There are several ways to do this.  svk/hg/git can be used to mirror
> svn, have local changes, then push when you are done.  You can use
> multiple svn checkouts, and redo your changes, one step at a time.
>
> <rant>
> I'm willing to overlook grouped commits some times.  However, I
> absolutely, positively, 100% *DETEST* "I can't remember what I did"
> type entries.  ARG!  If you don't know how to use the basic features
> of the revision control tool, then find some other industry to work
> in.  *ALL* revision tools support listing of changes.  *ALL*.
> </rant>
>
> --
>
> There is an old axiom, that states that methods(functions,
> subroutines, whatever they are called nowdays) should not be longer
> than a page.  While this doesn't completely hold nowadays(this came
> about during the physical printout era), the underlying principle
> still applies.  Smaller is easier to understand.  This goes for line
> lengths, methods, files(classes), and even to changesets.
>
> Also, one of the criteria I use in evaluating software, is how easy
> *non-associated* people have at fixing the problems that come up.
> Software has bugs; this is just the nature of the business.  And the
> bugs that tend to come up in production are the ones that occur
> *years* after deployment, after you've moved on to other things.
>
> Years after the deploy has occurred, you don't remember the little
> details; sometimes, you can't even recall the big ones.  And when
> trying to track down the *cause* of a problem, it generally is trying
> to find out *when* something broke.  Proper changelogs are the first
> step in figure out the when.
>