On wiki.blender.org, click on Code Review in the sidebar.

On Tue, Dec 29, 2020, 02:22 amdbcg via Bf-committers <
bf-committers@blender.org> wrote:

> I'm new to this. Where is the procedure for code review?
>
> On Mon, Dec 28, 2020 at 6:06 PM Ray Molenkamp via Bf-committers <
> bf-committers@blender.org> wrote:
>
> > I'd prefer if we excluded small commits completely, but willing to
> > settle on clarifying that the "smaller" commits still ought to be
> > the result of automated tools, If there is no clear indication a
> > cleanup was automated the commit should not be added. ie a commit
> > "Cleanup: clang-tidy some-check" could still very much be a manual
> > cleanup of the warns exposed by clang-tidy and suspect to unintentional
> > functional changes.
> >
> > --Ray
> >
> > On 2020-12-28 5:01 p.m., Brecht Van Lommel via Bf-committers wrote:
> > > Hi Ankit,
> > >
> > > Please go through code review for all commits, unless you are a module
> > > owner or admin that is the policy.
> > >
> > > Note that there are guidelines in the file itself. We could document it
> > in
> > > the wiki too, but I'm not sure it's needed. Perhaps the wording can be
> > > clarified? Suggestions are welcome. The text is as follows:
> > >
> > > # Changes that belong here:
> > > # - Massive comment, doxy-sections, or spelling corrections.
> > > # - Clang-format, PEP8 or other automated changes which are *strictly*
> > "no
> > > functional change".
> > > # - Several smaller commits should be added to this list at once,
> because
> > > adding
> > > #   one extra commit (to edit this file) after every small cleanup is
> > noisy.
> > >
> > > Note that only massive or automated changes are mentioned as belonging
> > > here, nothing else. And commits like these have functional changes:
> > >
> > > # Fix T82520: error building freestyle with Python3.8
> > > # Build-system: Force C linkage for all DNA type headers
> > > # Cleanup: use preprocessor version check for PyTypeObject declaration
> > >
> > > So as far as I can tell this is just not following the documented
> > > guidelines.
> > >
> > > Thanks,
> > > Brecht.
> > >
> > >
> > >
> > > On Mon, Dec 28, 2020 at 8:48 PM Ankit via Bf-committers <
> > > bf-committers@blender.org> wrote:
> > >
> > >> Hello
> > >> I'm getting used to it.
> > >> I'll remove several commits soon, now that I have received the
> > >> feedback on the last commit, and will use stricter conditions in the
> > >> future.
> > >>
> > >> My premise
> > >> - for being lenient was that it saves other people from committing to
> > >>   the file which keeps the noise in git log low.
> > >> - for renames was that a refactoring tool was used
> > >>   with as little human intervention as possible.
> > >> - for clang-tidy being the auto-fixes feature that clang-tidy gives.
> > >>
> > >>> All,
> > >> That's a new way of raising concerns on commits.
> > >>
> > >>> but I really
> > >>> would still like to see smaller and clearly manual cleanups
> > >>> in git blame.
> > >> If I see "cleanup" in the title, the onus is on the committer to make
> > >> sure that it really is a cleanup. If that promise is kept, I don't see
> > >> why a cleanup commit interests you.
> > >>
> > >>> Changes in this file don't even seem to go through code review.
> > >> I thought it keeps the overhead of keeping a utility file low.
> > >>
> > >> Ankit
> > >>
> > >>
> > >>> On 29-Dec-2020, at 00:40, Ray Molenkamp via Bf-committers <
> > >> bf-committers@blender.org> wrote:
> > >>> All,
> > >>>
> > >>> What's going in with this file? there's 50+ commits in there
> > >>> and I disagree with virtually every single hash I sampled
> > >>> from it.
> > >>>
> > >>> If we want to hide large changes made by automated tools like
> > >>> the big clang-format [1] change, yeah awesome, but I really
> > >>> would still like to see smaller and clearly manual cleanups
> > >>> in git blame.
> > >>>
> > >>> Changes in this file don't even seem to go through code review.
> > >>> In the original review [2] both Brecht and Campbell mention
> > >>> that it be "OK for larger changes" but that is not what appears to
> > >>> be happening.
> > >>>
> > >>> This is not how this is supposed to work, is it?
> > >>>
> > >>> --Ray
> > >>>
> > >>> [1]
> > >>
> > https://developer.blender.org/rBe12c08e8d170b7ca40f204a5b0423c23a9fbc2c1
> > >>> [2] https://developer.blender.org/D9234
> > >>> _______________________________________________
> > >>> Bf-committers mailing list
> > >>> Bf-committers@blender.org
> > >>> https://lists.blender.org/mailman/listinfo/bf-committers
> > >> _______________________________________________
> > >> Bf-committers mailing list
> > >> Bf-committers@blender.org
> > >> https://lists.blender.org/mailman/listinfo/bf-committers
> > >>
> > > _______________________________________________
> > > Bf-committers mailing list
> > > Bf-committers@blender.org
> > > https://lists.blender.org/mailman/listinfo/bf-committers
> > _______________________________________________
> > Bf-committers mailing list
> > Bf-committers@blender.org
> > https://lists.blender.org/mailman/listinfo/bf-committers
> >
> _______________________________________________
> Bf-committers mailing list
> Bf-committers@blender.org
> https://lists.blender.org/mailman/listinfo/bf-committers
>
_______________________________________________
Bf-committers mailing list
Bf-committers@blender.org
https://lists.blender.org/mailman/listinfo/bf-committers

Reply via email to