+1 to update the PR template. I think the intent is to ask PR authors to call out all the user-facing changes that need attention from the end users, such as new features and behavior changes, but doc change is clearly not one of them.
On Fri, Jan 17, 2025 at 7:10 AM Gengliang Wang <ltn...@gmail.com> wrote: > Thanks for pointing it out! Based on the discussion, I’ve created a PR: > https://github.com/apache/spark/pull/49534. > > Let me know what you think! > > On Thu, Jan 16, 2025 at 2:25 PM Xiao Li <lix...@databricks.com.invalid> > wrote: > >> Thank you for pointing it out! Let’s update the template to exclude >> documentation changes from the behavior change question. >> >> At the same time, I strongly believe that all behavior changes should be >> clearly documented in the PR description. For instance, in the first >> example PR https://github.com/apache/spark/pull/47756, the behavior >> changes were not documented. It’s crucial for all committers to carefully >> review the PR titles and descriptions to ensure they are accurate and >> complete before merging. >> >> How can we bring more attention to this issue and ensure it becomes a >> consistent practice? >> >> On Thu, Jan 16, 2025 at 1:54 PM Reynold Xin <r...@databricks.com.invalid> >> wrote: >> >>> Seems like we should fix the template if that's not the intent .... >>> >>> On Thu, Jan 16, 2025 at 1:52 PM Nicholas Chammas < >>> nicholas.cham...@gmail.com> wrote: >>> >>>> The template says "including all aspects such as the documentation fix >>>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36>.” >>>> The original intent may well have been about behavior changes only, but >>>> that’s not reflected in the current text of the PR template. >>>> >>>> >>>> On Jan 16, 2025, at 2:32 PM, Dongjoon Hyun <dongjoon.h...@gmail.com> >>>> wrote: >>>> >>>> The original intent is a user-facing *behavior* change technically >>>> which is the same with Apache Spark migration guide. >>>> >>>> If so, does it make sense to you? >>>> >>>> Probably, since the template was short to be concise, it could be >>>> interpreted in more ways than we thought. >>>> >>>> Dongjoon. >>>> >>>> On Thu, Jan 16, 2025 at 11:16 AM Nicholas Chammas < >>>> nicholas.cham...@gmail.com> wrote: >>>> >>>>> I didn’t write the pull request template and I am not sure about the >>>>> author’s original intent. >>>>> >>>>> However, the plain meaning of “any” — with the added emphasis >>>>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36> >>>>> as >>>>> well — suggests that the author of this policy did mean that yes, even a >>>>> typo fix in a user-facing documentation page merits a “yes” response to >>>>> this question. >>>>> >>>>> It’s strict but it’s also clear and unambiguous. No one has to think >>>>> about whether their user-facing change is big enough to merit a yes. >>>>> >>>>> IMO this is better than a more ambiguous policy that says something >>>>> like “yes if it’s user-facing _and_ is not a trivial fix”, because then >>>>> there will be disagreements about what is trivial vs. not. >>>>> >>>>> But my opinion on the correct policy doesn’t matter that much since >>>>> I’m not a committer and not a heavy reviewer. I mostly wanted to raise >>>>> this >>>>> so that we could somehow resolve this apparent disconnect between the >>>>> plain >>>>> meaning of the PR template and how PR authors are responding to it. >>>>> >>>>> >>>>> On Jan 16, 2025, at 2:02 PM, Dongjoon Hyun <dongjoon.h...@gmail.com> >>>>> wrote: >>>>> >>>>> I understand your concern, Nicholas. However, isn't it too strict? >>>>> >>>>> For the above example, adding a new HTML page is a user-facing change. >>>>> >>>>> https://github.com/apache/spark/pull/48852 (This is a new doc) >>>>> [SPARK-50309][DOCS] Document SQL Pipe Syntax >>>>> >>>>> https://github.com/apache/spark/pull/49098 (This is one line link >>>>> addition) >>>>> [SPARK-48426][DOCS][FOLLOWUP] Add `Operators` page to `sql-ref.md` >>>>> >>>>> Given your definition, even a word typo fix inside an HTML page >>>>> becomes a user-facing change. >>>>> Did I understand your definition correctly? Or, is it something else? >>>>> >>>>> Dongjoon. >>>>> >>>>> >>>>> On Thu, Jan 16, 2025 at 9:15 AM Nicholas Chammas < >>>>> nicholas.cham...@gmail.com> wrote: >>>>> >>>>>> This is not a big deal at all, but I figure it’s worth bringing up >>>>>> briefly because the pull request template does emphasize >>>>>> <https://github.com/apache/spark/blob/ffb31565e5af6f9ab2f8f7b500fbd595c8bb5a58/.github/PULL_REQUEST_TEMPLATE#L34-L36> >>>>>> : >>>>>> >>>>>> > ### Does this PR introduce _any_ user-facing change? >>>>>> > >>>>>> > Note that it means *any* user-facing change including all aspects >>>>>> such as the documentation fix. >>>>>> >>>>>> And yet I regularly see PRs where the author states “no user-facing >>>>>> change” even though there are clearly user-facing documentation changes >>>>>> included in the diff. >>>>>> >>>>>> Some examples: >>>>>> - https://github.com/apache/spark/pull/47756 >>>>>> - https://github.com/apache/spark/pull/49098 >>>>>> - https://github.com/apache/spark/pull/48852 >>>>>> - https://github.com/apache/spark/pull/49273 >>>>>> >>>>>> Again, not a big deal. But I assume committers care about this since >>>>>> it’s in the PR template. >>>>>> >>>>>> We should remind contributors (and each other) to answer this >>>>>> question correctly when appropriate, or we should perhaps update the PR >>>>>> template if the guidance there is outdated. >>>>>> >>>>>> Nick >>>>>> >>>>>> >>>>> >>>> >> >> -- >> >>
