Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Fri, Mar 02, 2018 at 12:05:03PM +0300, Dan Carpenter wrote:
> There are so many rules for kernel developers to deal with.  Is it worse
> to go over the 80 character limit or align the parameters properly?  Is
> it OK to start the subject with a lower case letter?  I get in trouble
> for using the wrong prefix but I'm the first person to patch a driver so
> how on earth am I supposed to read into your @#$R@#$@#$@#$@ mind to see
> what prefix you are going to want?
> 
> The imperitive thing is a good suggestion for people to think about but
> it's not something that you should suggest to other people out of the
> blue for no reason.  I've seen people do it for OPW and I'm like, "Eh...
> I guess in that case maybe the intern will have to deal with some super
> anal maintainer so they should know all the rules".  But generally,
> unless the person asked you to tutor them about every single unpleasant
> thing/maintainer they might have to deal then just let it be.  Otherwise
> you risk becoming that unpleasant thing they have to deal with.
> 
> Don't lose track of the bigger picture which is "Can you understand the
> changelog?"  There is no such thing as a perfect patch.  This patch has
> a style violation which I overlooked because it just doesn't matter.
> 
> It's discouraging to be nagged at.
> 
> regards,
> dan carpenter

No worries Dan, thanks for pointing this out.  I actually don't enjoy
reviewing staging patches.  I guessed no one really likes reviewing them
but I feel that we should all give back a little and since I don't have
all that much to give I figured reviewing staging patches was a good way
to do something that others probably don't like doing.  I'd like to be
able to do the reviews in a way that is encouraging to new devs but like
most I only have my own likes/dislikes to go on.  I liked having every
detail of my first X patches picked apart because it meant I was
confident then to go on and try to do patches outside of staging -
others may vary and I definitely agree with you that no one likes to be
nagged so it's a fine line.  If you have any suggestions for me as to
how to be more useful in reviewing staging patches please do share
them.  So far I only have one clue as to how to go about it and that was
this from Greg KH 'Just be polite'.  There certainly is a lot to learn
getting started with kernel work but for me anyways staging is doing
really well at smoothing the process and that is mainly thanks to Greg
and yourself in doing the reviews.  If I'm totally wrong and you two
really love doing it then I will happily bow out and not touch another
staging patch (please say so if this is the case).  If I can help to
carry the load then I'm happy to do so because you two helped me a lot.
Please continue to pick me up if I do wrongs - I'm super happy when you
do because then I've got a chance of doing it right next time :)


thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

And no top posting on email threads...  So don't you forget it.  :P

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

There are so many rules for kernel developers to deal with.  Is it worse
to go over the 80 character limit or align the parameters properly?  Is
it OK to start the subject with a lower case letter?  I get in trouble
for using the wrong prefix but I'm the first person to patch a driver so
how on earth am I supposed to read into your @#$R@#$@#$@#$@ mind to see
what prefix you are going to want?

The imperitive thing is a good suggestion for people to think about but
it's not something that you should suggest to other people out of the
blue for no reason.  I've seen people do it for OPW and I'm like, "Eh...
I guess in that case maybe the intern will have to deal with some super
anal maintainer so they should know all the rules".  But generally,
unless the person asked you to tutor them about every single unpleasant
thing/maintainer they might have to deal then just let it be.  Otherwise
you risk becoming that unpleasant thing they have to deal with.

Don't lose track of the bigger picture which is "Can you understand the
changelog?"  There is no such thing as a perfect patch.  This patch has
a style violation which I overlooked because it just doesn't matter.

It's discouraging to be nagged at.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 05:28:26PM -0800, Quytelda Kahja wrote:
> Tobin,
> I understand your point, and I've read submitting-patches.rst.  I made
> that wording choice because I was looking at some older commits that
> were worded like that.  I'm fairly new to the kernel workflow, so I
> was just trying to emulate something established, and it sounded less
> stilted than my imperative attempts.  However, I will word my change
> logs in the imperative as best I can in the future.

If you master the art of writing commit logs let me know - I'm chasing
that elusive goal myself.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Quytelda Kahja]

Tobin,
I understand your point, and I've read submitting-patches.rst.  I made
that wording choice because I was looking at some older commits that
were worded like that.  I'm fairly new to the kernel workflow, so I
was just trying to emulate something established, and it sounded less
stilted than my imperative attempts.  However, I will word my change
logs in the imperative as best I can in the future.

Thank you,
Quytelda Kahja

On Thu, Mar 1, 2018 at 12:54 PM, Tobin C. Harding  wrote:
> On Thu, Mar 01, 2018 at 02:15:00PM +0300, Dan Carpenter wrote:
>> On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
>> > On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
>> > > The code that generates a WLAN capability mask is repeated in five
>> > > functions.  This change refactors that code into a new function, which is
>> > > called now in each of those functions.
>> >
>> > Perhaps in future something like:
>> >
>> > Code to generate the WLAN capability mask is duplicated five times
>> >
>> > Add helper function to generate WLAN capability mask, refactor code to
>> > use newly defined function.
>> >
>>
>> I honestly don't see the difference between that and what Quytelda
>> wrote?  I understood the original changelog just fine.
>>
>> regards,
>> dan carpenter
>
> I had a feeling that the sentiment of the suggestion I was trying to get
> at didn't come across, thanks for pointing it out.  I was intending to
> suggest not using sentences like this
>
> > This change refactors that code into a new function, which is
> > called now in each of those functions.
>
> And instead use, as suggested in submitting-patches.rst, imperative mood
>
> Refactor code into new function ...
>
> FTR I find the English bits of kernel dev (and programming in general)
> the most difficult even though English is my first language.  I would
> like to write it better.
>
>
> Hope this helps,
> Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 02:15:00PM +0300, Dan Carpenter wrote:
> On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
> > On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> > > The code that generates a WLAN capability mask is repeated in five
> > > functions.  This change refactors that code into a new function, which is
> > > called now in each of those functions.
> > 
> > Perhaps in future something like:
> > 
> > Code to generate the WLAN capability mask is duplicated five times
> > 
> > Add helper function to generate WLAN capability mask, refactor code to
> > use newly defined function.
> > 
> 
> I honestly don't see the difference between that and what Quytelda
> wrote?  I understood the original changelog just fine.
> 
> regards,
> dan carpenter

I had a feeling that the sentiment of the suggestion I was trying to get
at didn't come across, thanks for pointing it out.  I was intending to
suggest not using sentences like this

> This change refactors that code into a new function, which is
> called now in each of those functions.

And instead use, as suggested in submitting-patches.rst, imperative mood

Refactor code into new function ...

FTR I find the English bits of kernel dev (and programming in general)
the most difficult even though English is my first language.  I would
like to write it better.


Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
> On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> > The code that generates a WLAN capability mask is repeated in five
> > functions.  This change refactors that code into a new function, which is
> > called now in each of those functions.
> 
> Perhaps in future something like:
> 
> Code to generate the WLAN capability mask is duplicated five times
> 
> Add helper function to generate WLAN capability mask, refactor code to
> use newly defined function.
> 

I honestly don't see the difference between that and what Quytelda
wrote?  I understood the original changelog just fine.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> The code that generates a WLAN capability mask is repeated in five
> functions.  This change refactors that code into a new function, which is
> called now in each of those functions.

Perhaps in future something like:

Code to generate the WLAN capability mask is duplicated five times

Add helper function to generate WLAN capability mask, refactor code to
use newly defined function.


See Documentation/process/submitting-patches.rst section 2 for more info.

2) Describe your changes

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel