Default check branch should be "origin/main"

[saroad2]

Description

In the past few years the use of the terminology "master" as the main branch is no longer acceptable.
GIthub, along with other companies, use the word "main" instead, so the main branch is called "origin/main".

With that spirit, I changed the default check branch for the towncrier check command to be "origin/main".

Checklist

• Make sure changes are covered by existing or new tests.
• For at least one Python version, make sure local test run is green.
• Create a file in src/towncrier/newsfragments/. Describe your
  change and include important information. Your change will be included in the public release notes.
• Make sure all GitHub Actions checks are green (they are automatically checking all of the above).

[codecov]

Codecov Report

Merging #400 (affa1a2) into trunk (6525465) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            trunk     #400      +/-   ##
==========================================
+ Coverage   97.85%   97.91%   +0.05%     
==========================================
  Files          22       22              
  Lines        1400     1440      +40     
  Branches      130      135       +5     
==========================================
+ Hits         1370     1410      +40     
  Misses         15       15              
  Partials       15       15              

Impacted Files	Coverage Δ
src/towncrier/check.py	95.45% <100.00%> (+1.33%)	⬆️
src/towncrier/test/test_check.py	99.34% <100.00%> (+0.12%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[adiroiban]

Thanks for the PR.

I am not a fan of "origin/master" but this change will break backward compatibility.

If we are going to push this, we need a big warning as part of the release notes.
I don't think that a .misc is enough.

What I can think of, is creating a different command, other than 'check' that can be used by new users.

Maybe it can be called "verify".
It will do the same thing as check, but using origin/master as the default.

And emit a deprecation warning when the "check" command is used.

I know that this is a lot of trouble ... so I don't know.

This is about pleasing new users vs adding extra work for existing users.

I would say that is easier for a new user to explicitly add the branch, than for an existing user to have a process failing, after years in which all was ok.

So I don't know. I am not +1 for this.

@altendky @hynek sorry for the noise.
Can you help see if this PR should be merged?

A +1 or -1 should be enough. No need for extra arguments.

Thanks!

[saroad2]

This PR is not about pleasing new users. It is about that "master" is an offensive word against a lot of communities around the world. This word needs to be removed from our terminology as programmers and people.

The change is easy: for now, we can keep using the "origin/master" as the default check branch, but add a deprecation warning that in the next major version it will be changed to "origin/main".
When you publish the next major version, replace "origin/master" with "origin/main".

A new towncrier verify command is a bad idea. No need for 2 commands that do the same thing.

As for the news fragment type, what would be a better option than "misc"? "feature"?

[hynek]

How about checking if main exists, falling back to master (with deprecation warning)?

[altendky]

To satisfy the requested boolean response to the PR as is... 👎 but I also feels that's unnecessarily negative about making some change here.

The hazards described about just changing this make sense so I think we should at least think through possibilities to improve it, as we are. The fallback idea seems reasonable. Is there a way we can make it a generally useful thing instead of being exclusively for dealing with the old and new git defaults? Would it being a list be sensible and... also useful for... there has to be another use case. :]

Or maybe it really is simplest and cleanest to not migrate towards more complex but rather to just migrate to no default. You are required to either specify the option or set it in the config. Or, option wins if specified and falls back to the config. If we need to get rid of the word then we can do so by just removing the default someday without picking a new winner. I guess the action in this case would be 1) deprecate calling check without either the option or config entry and then 2) at some point remove the default.

The news fragment can be figured out once we decide the path forward.

[adiroiban]

Many thanks for your feedback.

I am happy to see "origin/main" as the new default, with "origin/master" as a fallback.
I know it's more work... and maybe it doesn't help, as the original intention was to purge the concept of "master".

I like zero configuration / it just works (tm) so... but towncrier is something that you set up once in your CI or Makefile or buildtool and forget about it...so always requiring to pass the compare branch, is not a big deal... but maybe "origin/main" as default with fallback to "origin/master" is less work.

This can go with a ".removal" (Removal and deprecation) release note.

If you have both "origin/main" and "origin/master" you might not get what you want with the default behaviour.

---

for any new repo I go with "origin/main" and switch existing when feasible.

I was the one switching towncrier from "origin/master" to "origin/trunk" as the main branch.

But for this PR, I think that we need at least a ".removal" note and fallback to "origin/master" to not generate unnecessarily extra work for end users.

[hynek]

My second take would've been also to remove the default altogether. Everybody who has main already has to pass it anyway.

It would be nice if it could become a config option.

[adiroiban]

Thanks @saroad2 for the updates. Let us know when this is ready for review.

As long as the non-code coverage tests are green, I think that this should be ready for merge.

Having some automated tests would be great, but at this point, since towncrier has very few contributors, I would leave the automated tests as options.

It is sad that we can't block a PR on missing automated tests, but I think that for open source project this is the right thing to do.

If people care about preventing regressions, then this is open source and anyone can submit automated tests to help reduce the regressions.

If someone cares enough for a feature and doesn't want that functionality to fail in the future, a set of automated test should be provided.
If someone sends a PR without tests, it should expect and plan that functionality might stop working in future releases.

[adiroiban]

there is a conflict, since I have merged another PR in which the help text for the CLI is extended

[saroad2]

Hey @adiroiban .

So I merged trunk into this branch and fixed the conflicts.

As for testing, it is impossible to test the new behavior with the current way the tests are written. As for now, we test the towncrier check command by creating an actual git repository and validating what's happening when comparing with a branch. This repository has no origin remote, so I cannot test the fallback to origin/main or origin/master.

There are 2 things we can do:

1. Leave the new feature untested, since we cannot test it with the current way the tests are written
2. Rewrite the towncrier check tests to use stubs (aka. mocks) to replace the usage of a real git repository.

If we choose option 2, I'll open a new PR just for that.

[hynek]

Wouldn’t it be possible/simpler to add an origin?

[saroad2]

Hey @hynek ,

It will be possible to do that, but it won't be simpler because now I have to create 2 repositories for each test and define one of them as origin.
This makes the code messier and longer. I think using stubs is the right way to go.

Your choice. If you want to keep the usage of real git, I'll write the new tests

[adiroiban]

I guess that the existing tests can be left as they are now.

A new tests can be added with its own setup and repo.
I guess that having a remote that is a local path is ok and all operation should work

git remote add origin /temp/path/to/test/git/origin/repo/.git

[adiroiban]

@saroad2 let me know if you want to have the PR merged without tests or I should wait for the tests.

[saroad2]

Hey @adiroiban ,

I'm a little bit preoccupied right now, so I won't have time soon to add the unit tests.
If you want, you can edit this PR and add the relevant tests.

[hynek]

So it seems indeed to be problematic to just add non-existent origin branches, so I've implemented it using monkeypatching. I'll look at the other missing coverage next time I have time.

[hynek]

Looks like I already caught them all!

[adiroiban]

Ok. Thanks.

let's merge it.
I don't have time to run manual tests for this.
I hope all is ok.

if there are any issues, we can come back and fix it and write better tests if required.