[code quality] - Pure formatting PRs possibly harmful?
133 views
Skip to first unread message
James Crist
Apr 14, 2015, 1:54:00 PM4/14/15
to sy...@googlegroups.com
I expect this to turn into a flamewar, so I'm going to ask everyone to only give one opinion (voting style). Please please please don't fight about this.
I just got back from PyCon, and there was a presentation there on pep 8 formatting, and one of the main points was that pure formatting PRs mess up the history (git blame no longer shows who last made a non-trivial change). The proposed solution would be to only fix formatting in areas where you're also making a non-trivial change.
For example, say I make a tiny bug fix in function foo - I could also clean up some of the code in foo. That way the last person to touch foo is not someone who added a space between an operator, but someone who actually changed the functionality of foo.
This is not to bash people who do formatting fixes - they are highly valuable, it's just saying that formatting (IMO) should be done incrementally as we improve the codebase, not as just formatting PRs. (docs are an exemption from this, as formatting can be thought of as a functional change).
Thoughts???
I just got back from PyCon, and there was a presentation there on pep 8 formatting, and one of the main points was that pure formatting PRs mess up the history (git blame no longer shows who last made a non-trivial change). The proposed solution would be to only fix formatting in areas where you're also making a non-trivial change.
For example, say I make a tiny bug fix in function foo - I could also clean up some of the code in foo. That way the last person to touch foo is not someone who added a space between an operator, but someone who actually changed the functionality of foo.
This is not to bash people who do formatting fixes - they are highly valuable, it's just saying that formatting (IMO) should be done incrementally as we improve the codebase, not as just formatting PRs. (docs are an exemption from this, as formatting can be thought of as a functional change).
Thoughts???
Jason Moore
Apr 14, 2015, 2:04:22 PM4/14/15
to sy...@googlegroups.com
I watched Raymond's talk but didn't come away thinking that we should merge formatting and functionality edits. That seems to cloud the history even more. With git blame you can see the commit message and if it says "formatting", then you need to git blame before that commit to see who actually wrote it.
I think the main take away from the talk is just "don't be super strict with pep8". Which he could've said in a much briefer way.
--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.
To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/5fe1f28a-9ac0-42db-882a-b8ed0a0f5080%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Aaron Meurer
Apr 14, 2015, 3:02:29 PM4/14/15
to sy...@googlegroups.com
Have you tried git blame -CCC -M? According to the manpage it is
supposed to be smarter about stuff. There is also the -w flag which
makes it ignore whitespace (I haven't really tested it, though). In
general, git blame only gets you to the last change of a line. You
typically have to checkout the commit before the one given and repeat.
It would be nice if there were an easier way to do this.
In general, though, I'm not a fan of changing the way we work on code
just for the sake of git. git is a tool. Smarter tools can exist, like
a smarter git blame that gets to the real source of something. I've
always felt that git's pickaxe feature deserves a more usable frontent
than git blame and git log -S.
I'm much more concerned about how things like pep8 affect people's
workflow, and especially how the affect new contributors (like if we
have really strict pep8 rules, then does it raise the barrier to
entry? Or how does poorly formatted code affect the ability for people
to read it?).
Aaron Meurer
> https://groups.google.com/d/msgid/sympy/CAP7f1AgAgvD0u0hq8YjQjnrXX7yaqXPMQNBUOa6zFukbNjM%3D%3Dg%40mail.gmail.com.
supposed to be smarter about stuff. There is also the -w flag which
makes it ignore whitespace (I haven't really tested it, though). In
general, git blame only gets you to the last change of a line. You
typically have to checkout the commit before the one given and repeat.
It would be nice if there were an easier way to do this.
In general, though, I'm not a fan of changing the way we work on code
just for the sake of git. git is a tool. Smarter tools can exist, like
a smarter git blame that gets to the real source of something. I've
always felt that git's pickaxe feature deserves a more usable frontent
than git blame and git log -S.
I'm much more concerned about how things like pep8 affect people's
workflow, and especially how the affect new contributors (like if we
have really strict pep8 rules, then does it raise the barrier to
entry? Or how does poorly formatted code affect the ability for people
to read it?).
Aaron Meurer
Ondřej Čertík
Apr 14, 2015, 4:01:01 PM4/14/15
to sympy
On Tue, Apr 14, 2015 at 1:02 PM, Aaron Meurer <asme...@gmail.com> wrote:
> Have you tried git blame -CCC -M? According to the manpage it is
> supposed to be smarter about stuff. There is also the -w flag which
> makes it ignore whitespace (I haven't really tested it, though). In
> general, git blame only gets you to the last change of a line. You
> typically have to checkout the commit before the one given and repeat.
> It would be nice if there were an easier way to do this.
Indeed, I was wondering if there is some automatic way to track
> Have you tried git blame -CCC -M? According to the manpage it is
> supposed to be smarter about stuff. There is also the -w flag which
> makes it ignore whitespace (I haven't really tested it, though). In
> general, git blame only gets you to the last change of a line. You
> typically have to checkout the commit before the one given and repeat.
> It would be nice if there were an easier way to do this.
changes across the code base, so you don't need to manually checkout
the one before commit and repeat.
Ondrej
Aaron Meurer
Apr 14, 2015, 4:09:09 PM4/14/15
to sy...@googlegroups.com
On Tue, Apr 14, 2015 at 3:00 PM, Ondřej Čertík <ondrej...@gmail.com> wrote:
> On Tue, Apr 14, 2015 at 1:02 PM, Aaron Meurer <asme...@gmail.com> wrote:
>> Have you tried git blame -CCC -M? According to the manpage it is
>> supposed to be smarter about stuff. There is also the -w flag which
>> makes it ignore whitespace (I haven't really tested it, though). In
>> general, git blame only gets you to the last change of a line. You
>> typically have to checkout the commit before the one given and repeat.
>> It would be nice if there were an easier way to do this.
>
> Indeed, I was wondering if there is some automatic way to track
> changes across the code base, so you don't need to manually checkout
> the one before commit and repeat.
Some interesting suggestions at
> On Tue, Apr 14, 2015 at 1:02 PM, Aaron Meurer <asme...@gmail.com> wrote:
>> Have you tried git blame -CCC -M? According to the manpage it is
>> supposed to be smarter about stuff. There is also the -w flag which
>> makes it ignore whitespace (I haven't really tested it, though). In
>> general, git blame only gets you to the last change of a line. You
>> typically have to checkout the commit before the one given and repeat.
>> It would be nice if there were an easier way to do this.
>
> Indeed, I was wondering if there is some automatic way to track
> changes across the code base, so you don't need to manually checkout
> the one before commit and repeat.
http://stackoverflow.com/questions/5098256/git-blame-prior-commits.
Aaron Meurer
Joachim Durchholz
Apr 14, 2015, 4:41:52 PM4/14/15
to sy...@googlegroups.com
Am 14.04.2015 um 19:54 schrieb James Crist:
> For example, say I make a tiny bug fix in function foo - I could also clean
> up some of the code in foo. That way the last person to touch foo is not
> someone who added a space between an operator, but someone who actually
> changed the functionality of foo.
+1
> For example, say I make a tiny bug fix in function foo - I could also clean
> up some of the code in foo. That way the last person to touch foo is not
> someone who added a space between an operator, but someone who actually
> changed the functionality of foo.
It's what I've been doing all the time.
Well, being human: *trying* to do all the time :-)
One exception: I don't apply that argument to adding or removing blank
lines, or where the change in functionality is more visible elsewhere
(e.g. when cleaning up import lists).
James Crist
Apr 14, 2015, 5:31:09 PM4/14/15
to sy...@googlegroups.com
> Have you tried git blame -CCC -M? According to the manpage it is supposed to be smarter about stuff.
I had not, but it looks better. I've rarely encountered this issue, I just thought I'd ask how the community felt about it. Personally, I still feel like cleaning up code should be done only around things the commiter just worked on, and in the same PR (not necessarily the same commit), but that's just a personal opinion. I agree that keeping the barrier to entry for SymPy as low as possible is *much* more important than things like this. Duly noted.
--
You received this message because you are subscribed to a topic in the Google Groups "sympy" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sympy/lVUoV2_Jzm0/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sympy+unsubscribe@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.
To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/552D7B86.1040902%40durchholz.org.
James Crist
Apr 14, 2015, 5:36:32 PM4/14/15
to sy...@googlegroups.com
Side tracking the conversation a bit, PyCon was very productive for me. I gave a talk to a group of Sage users, and they seemed excited about SymPy. Apparently (at least according to them) the symbolic support in sage is poor, and sympy is much better. It word be nice to get sympy/csympy to be the main symbolics engine for sage, as the increased user base would probably result in an increased development team.
Aaron Meurer
Apr 14, 2015, 5:41:01 PM4/14/15
to sy...@googlegroups.com
On Tue, Apr 14, 2015 at 4:36 PM, James Crist <cris...@umn.edu> wrote:
> Side tracking the conversation a bit, PyCon was very productive for me. I
> gave a talk to a group of Sage users, and they seemed excited about SymPy.
> Apparently (at least according to them) the symbolic support in sage is
> poor, and sympy is much better. It word be nice to get sympy/csympy to be
> the main symbolics engine for sage, as the increased user base would
> probably result in an increased development team.
That's great. We should focus on improving our symbolic capabilities
> Side tracking the conversation a bit, PyCon was very productive for me. I
> gave a talk to a group of Sage users, and they seemed excited about SymPy.
> Apparently (at least according to them) the symbolic support in sage is
> poor, and sympy is much better. It word be nice to get sympy/csympy to be
> the main symbolics engine for sage, as the increased user base would
> probably result in an increased development team.
(e.g., the integration algorithm). That's probably the most important
thing for them (well speed too, especially for core symbolics, but
Sage already puts up with some slow systems for things like
integration).
Aaron Meurer
>>> To post to this group, send email to sy...@googlegroups.com.
>>> Visit this group at http://groups.google.com/group/sympy.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/sympy/552D7B86.1040902%40durchholz.org.
>>>
>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>
> --
>>> Visit this group at http://groups.google.com/group/sympy.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/sympy/552D7B86.1040902%40durchholz.org.
>>>
>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/sympy/CAJ2L7mdwPXSy%2BsOH2b4wHuj4PWGw%2BV%2BDGEnEiZCWfchKUS-qOg%40mail.gmail.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
Ondřej Čertík
Apr 14, 2015, 7:20:30 PM4/14/15
to sympy
On Tue, Apr 14, 2015 at 3:36 PM, James Crist <cris...@umn.edu> wrote:
> Side tracking the conversation a bit, PyCon was very productive for me. I
> gave a talk to a group of Sage users, and they seemed excited about SymPy.
> Apparently (at least according to them) the symbolic support in sage is
> poor, and sympy is much better. It word be nice to get sympy/csympy to be
> the main symbolics engine for sage, as the increased user base would
> probably result in an increased development team.
Yes, that's exactly my plan, and there is a GSoC proposal this year
> Side tracking the conversation a bit, PyCon was very productive for me. I
> gave a talk to a group of Sage users, and they seemed excited about SymPy.
> Apparently (at least according to them) the symbolic support in sage is
> poor, and sympy is much better. It word be nice to get sympy/csympy to be
> the main symbolics engine for sage, as the increased user base would
> probably result in an increased development team.
from Isuru to do exactly that (i.e. work on csympy being the main
symbolic engine for Sage --- obviously this is a long term project, so
it's just a start).
I have no doubt that if the SymPy ecosystem contains a very fast core
(CSymPy) that is faster than other software, then people can consider
SymPy very seriously for their work. It's just a question to figure
out how to make it all work nicely together.
Ondrej
>>> To post to this group, send email to sy...@googlegroups.com.
>>> Visit this group at http://groups.google.com/group/sympy.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/sympy/552D7B86.1040902%40durchholz.org.
>>>
>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>
> --
>>> Visit this group at http://groups.google.com/group/sympy.
>>> To view this discussion on the web visit
>>> https://groups.google.com/d/msgid/sympy/552D7B86.1040902%40durchholz.org.
>>>
>>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/sympy/CAJ2L7mdwPXSy%2BsOH2b4wHuj4PWGw%2BV%2BDGEnEiZCWfchKUS-qOg%40mail.gmail.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
Reply all
Reply to author
Forward
0 new messages