Re: Change packaging preferences? Pull request threshold?

If your changes all relate to Python, I think that it's fine to have one
pull request. I wouldn't expect to have any particularly icky merge
conflicts, and interleaved commits have seldom caused problems for us.




On Mon, Aug 25, 2014 at 1:05 PM, Thomas Shinnick <[email protected]> wrote:

> I'm looking at doing numerous changes to python.c , several of which are
> concerning Python 3 compatibility, which is broken on around 10 different
> APIs.  (Mishandling strings using Python 2-style PyBytes_* APIs)
>
> But in researching/working on *those*, I find other bits broken.  Such as
> not handling ref counts correctly upon use of PySequence_GetItem(), which
> is an unusual API.  Or memory leaks apparently introduced to satisfy
> compiler warnings!  Or out-of-order Py_INCREF/PY_DECREF's.  Everytime I
> look closer, something says 'boo'.
>
> So one *topic* is Python3 compatibility, but that will be ~10 separate
> commits, one per API, as I really like to test/verify separately.  (Testing
> font.printSample() took ~250 lines of Python test code).  I wouldn't want
> to do one PR per API, but I don't know how long it will take to work
> through all of these - I wouldn't want to wait weeks to do one PR for all.
> (When's the next release target?)
>
> Another topic might be PySequence_GetItem() ref count issues, but those
> are scattered *everywhere*.  I don't think it is practical to do just one
> commit/PR.
>
> Then there are the Coverity botherances and -Wall notes and so on.
>
>
> What are your suggested guidelines for when to partition work into
> separate PRs?
> How long to wait before submitting "work-so-far" as a new batch of commits
> in a PR?
> How much 'change' is too much to review in one PR?
>
>
> I'd really hate to submit one huge PR with 47 commits after more than 3
> weeks, only to find out I actually got one commit horribly wrong and
> everything has to be backed out.  So... granularity, topic-relatedness,
> timeliness, ease-of-review, ....  hmmm?
>
>
> Two examples of totally incidental discoveries:
>
>
> https://github.com/fontforge/fontforge/blob/master/fontforge/python.c#L7820
>        7820         pt += strlen(pt);
>     s.b.
>                     pt += strlen(str);
>         but I haven't *proved* it yet...
>
> and
>
> https://github.com/fontforge/fontforge/blob/master/fontforge/python.c#L7786-L7790
>     What is this code 'for'?  Oops, the 'return' disappeared!
>         commit  39c2b7e7dc  rrthomas 2014-03-06  "Require Python >= 2.7"
>
> https://github.com/fontforge/fontforge/commit/39c2b7e7dc4c427cca01046e2b032acaccbd0a29#diff-bd864a9cbdf26fcfdc3dc905c7445c17
>
>
>
> ------------------------------------------------------------------------------
> Slashdot TV.
> Video for Nerds.  Stuff that matters.
> http://tv.slashdot.org/
> _______________________________________________
> fontforge-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/fontforge-devel
> http://fontforge.10959.n7.nabble.com/Developer-f3.html
>
>

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/

_______________________________________________
fontforge-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/fontforge-devel
http://fontforge.10959.n7.nabble.com/Developer-f3.html