Consider modifying an existing NEWS fragments as acceptable

[pradyunsg]

See pypa/pip#11879, which modifies a news/ fragment and thus does the correct effect on the changelog.

[webknjaz]

@pradyunsg does skip news address this need?
There's similar logic in place for "release PRs" but it checks for "fragments removed + target rendered changelog has lines added".

I'm not sure that "an old change note modified" is an equally acceptable marker of a change note presence in a PR.

Though, this could probably be hidden behind a toggle.

I just want to make sure that a generic solution we'd come up with is acceptable for other end-users, not just pip. WDYT? Any ideas?

[pradyunsg]

I think it's good to consider "modifed an existing changelog file" to be a reason to consider this check to be a success. It's the behaviour of towncrier check --compare-with origin/${{ github.base_ref }} FWIW.

If we do decide to add a toggle for this, I don't have a strong sentiment toward what the default of that toggle should be -- either option works.

[webknjaz]

Well, the default would be the current status quo so that it doesn't break existing users...

[webknjaz]

It's the behaviour of towncrier check --compare-with origin/${{ github.base_ref }} FWIW.

This wasn't what browntruck implemented, though. Hence the current behavior.

[pradyunsg]

The check in browntruck is "not removed" which is logically equivalent to "added or modified".

https://github.com/pypa/browntruck/blob/e9c430a162526a3e8e74e6915d16e986d965e111/browntruck/hooks/news.py#L81

https://github.com/matiasb/python-unidiff/blob/ab8fcd4fce0c5e75dd016e4f1df19faed1333810/unidiff/patch.py#L428

[pradyunsg]

IMO this entire section...

chronographer-github-app/chronographer/event_handlers.py

Lines 280 to 309 in 6f275b2

	news_fragments_added = [
	f for f in diff
	if f.is_added_file and _tc_fragment_re.search(f.path)
	]
	logger.info(
	'News fragments are %s',
	'present' if news_fragments_added
	else 'absent',
	)
	if news_fragments_added:
	labels_url = f'{pull_request["issue_url"]}/labels'
	await gh_api.post(
	labels_url,
	preview_api_version='symmetra',
	data={
	'labels': [
	LABEL_PROVIDED,
	],
	},
	)
	news_fragments_required = requires_changelog(
	diff,
	_tc_fragment_re,
	repo_config.get('paths', {}),
	towncrier_config=towncrier_config,
	)
	report_success = not news_fragments_required or news_fragments_added

can be replaced with...

news_fragments_added = [
    f for f in diff
    if _tc_fragment_re.search(f.path) and not f.is_removed_file
]
report_success = bool(news_fragments_added)

Although, I'd say news_fragments_changed rather than added. This also removes the complexity of needing to detect "is this a release PR".

[webknjaz]

Ah... It looks like I was trying to address #8 with 981b4ef. I kinda like this heuristic as default. It's more accurate to me so I'd rather go with a feature flag.

[pradyunsg]

For the release PR case, I think you can check if the destination changelog file itself has been added/modified. If it has been, that's maybe also good-enough?