Re: Patch submissions

>
> - If we commit half-baked code, there's no guarantee that the
> submitter will actually come back and fix the outstanding problems.
> We've had *significant* baggage in pgAdmin 3 in the past when this has
> happened.

I'm not suggesting you apply the patches before you feel the issues are
addressed. I'm just suggesting that the patches should not be merged
together.

Tira

On Mon, Mar 20, 2017 at 7:12 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Sat, Mar 18, 2017 at 4:03 PM, Atira Odhner <aodhner(at)pivotal(dot)io> wrote:
> >>
> >> Patch 4 fixed quotes in sqleditor.js (arguably part of feature 2)
> >
> >
> > This change isn't related to any of the other changes. Combining it with
> > another patch would make it more difficult to understand the project
> > history.
>
> OK.
>
> >> Patch 9 fixed patch 7 (feature 1)
> >
> >
> > This change was to exclude node modules from the dmg build. Yes, it is
> > *related* to adding node modules. I think having it as a separate commit
> > again makes history easier to understand. The commit message on the
> changes
> > is very clear and specific. The commit improves one simple thing which
> the
> > other commits work with or without.
>
> You could argue that one either way, I agree.
>
> > Regarding the commits that move around the specs, I understand the
> desire to
> > rewrite history and "pretend" that the code was right the first time
> around.
> > There are valid reasons for re-writing history, but it does more harm
> than
> > good when applied so liberally. It hides the conversations that were had
> > about the code. It's true that those conversations live on in an archived
> > email thread, but why hide them from the code history?
> > If one day someone wants to move karma.conf back up to the pgAdmin4
> > directory, having two commits instead of one will provide valuable
> > information to them. Just annotating the file will show them that there
> has
> > already been a conversation about where karma.conf lives and there is
> > probably some good thought behind it. Having that context will tip them
> off
> > that they should then go look at the email thread.
>
> There are at least 3 issues (putting aside the issue of it being a
> pain to review incremental patches with intermixed and unrelated
> changes):
>
> - You're trying to fundamentally change the way we, and the greater
> PostgreSQL project and eco-system have worked very successfully for
> nearly 20 years.
>
> - We want the tree to be clean and as close to ready-for-release at
> any time and from any commit. Patches that are rejected are usually
> done so for reasons that would make it undesirable to release the code
> with that change, and/or because those changes will break something,
> often resulting in build/test failures in CI.
>
> - If we commit half-baked code, there's no guarantee that the
> submitter will actually come back and fix the outstanding problems.
> We've had *significant* baggage in pgAdmin 3 in the past when this has
> happened.
>
> > There is no final version of code. That's why it's so important to write
> > code that is readily changeable, and that's why it's important to have a
> > commit history that's understandable as well.
>
> Of course there's no final version - but what we commit, we expect to
> be of release quality, i.e. having no known issues.
>
> > Dave, I absolutely appreciate the work that you are doing to review our
> > commits before pulling them in. You've provided lots of great substantive
> > feedback which clearly involved carefully reading the code as well as
> > testing out our changes.
> > I'd like to lend a hand in reviewing the patches from your team in order
> to
> > alleviate some of this process burden from you. Is there a process I can
> > follow beyond replying to a patch email?
>
> That would certainly be a huge help. There are some review notes in
> the docs at https://www.pgadmin.org/docs4/1.x/code_review.html, which
> are really just reminders of some specific things to check. The sort
> of general topics we're checking when reviewing include:
>
> - Does the change do what is intended?
> - Do UI changes follow established patterns?
> - Are there any potential UX issues in the design?
> - Are there any unexpected or undesirable side effects?
> - Does the code follow our standards, and is it designed in a sensible way?
> - Will we be able to understand and maintain this code in 10 years time?
> - Do regression tests pass?
> - Are regression tests added (where appropriate)?
> - Has the documentation been updated (where appropriate)?
>
> Many key points from
> https://wiki.postgresql.org/wiki/Reviewing_a_Patch are also relevant
> with pgAdmin.
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>