Fix an AppVeyor environment variable being null because it does not exist.

[teo-tsirpanis]

This PR mitigates appveyor/ci#2939.

[matthid]

Isn't null the "correct" value if it indeed doesn't exist?
I think this is kind of related to #2338

Whatever we decide to do it should be consistent. So either we use null everywhere or we never make a difference between empty and missing variables. This would mean changing the underlying API instead?

[teo-tsirpanis]

The problem with null in a library-provided value is that the user does not reasonably expect it being null.

The issue is not about environVarOrNone. It's about AppVeyor.Environment.RepoCommitMessageExtended being null sometimes. The origin of that value being in an environment variable is a detail.

We could return None when the commit message is made of one line only. But the only way the Appveyor's value would have been equal to the empty string is when the commit message is made of only one line, and a newline afterwards. Quite the abnormat scenario to cater for.

[matthid]

I understand - I'm taking about changing environVar globally instead of the proposed patch, which internally changes environVarOrNone and the linked issue.

[github-actions]

There has not been any activity in this pull request for the last 3 months so it will be closed in 14 days if there is no activity.

[teo-tsirpanis]

@matthid, any news with this one?

[matthid]

My points discussed above are still open?

[matthid]

To clarify: If we decide that null is not the correct thing to do here. Why would it be correct in the underlying API?

[teo-tsirpanis]

In AppVeyor, the environment variable is missing. We obviously cannot do something like that here. And we are not even sure whether its behavior is "correct". This was what I had said in their forum:

Given that environment variables represent important configuration data, that there is no other variable that may not exist, and that AppVeyor's documentation on environment variables mentions in the beginning "[e]nvironment variables that are set by AppVeyor for every build", my proposal is to always define the APPVEYOR_REPO_COMMIT_MESSAGE_EXTENDED variable, setting it to an empty string by default.

The discussion has been stale, as well as the GitHub issue I had opened at AppVeyor. I don't think we should be waiting for them.

[matthid]

I think I slowly understand what you are trying to say. But now it is wrong the other way around: If you are not on AppVeyor the variable will be empty instead of null.
I can live with that, but just to ensure I understand it correctly: That would be the correct thing to do following your argument, correct?

[teo-tsirpanis]

Regarding reading that value while not on AppVeyor, it should be assumed that the build script first checks the running build server. If it still does however, these CI-specific values return the dreaded null, but that's another story. I don't think we should care that this specific value is not null if retrieved when running under build server other than AppVeyor.

[matthid]

Yes, I think I got it now. Thanks for explaining.