ionide / KeepAChangelog

Parser and MSBuild Targets for using Keep-A-Changelog-formatted Changelogs in your Build
MIT License
28 stars 4 forks source link

Make the process fail if the changelog file is not found #12

Open MangelMaxime opened 2 years ago

MangelMaxime commented 2 years ago

Hello @baronfel, this is an issue created from our discussion on F# slack.

Currently, if the changelog file is not found there is no indication for the user. The dotnet pack command succeed as if nothing was wrong.

I personally think that if the changelog file is not found at the specified path, the process should fail because it is generating something from an invalid context.

For example, when we specify the PackageLicenseFile node in an fsproj the task is failing if the provided value is invalid or the file is missing/not found.

baronfel commented 2 years ago

I think this is a good idea, but I think it should be a warning by default, mostly because the design of this library is 'implicit' by default. Let's dive into why.

When you add PackageLicenseFile to a project file, you had to take an explicit action in order to opt in to using that feature. That tells the MSBuild target author that you explicitly want to use the License File feature, so they can be quite strict.

This library by comparison currently has no such opt-in requirement - it implicitly discovers and uses a CHANGELOG file in the current project directory if one exists. This means that there's no user action we can point to to justify a fail-by-default behavior in my opinion.

There are two ways we could address this IMO:

MangelMaxime commented 2 years ago

Thank you for the clear explanations.

Personally, I always preferred explicit behaviour from my code and so would prefer the second option.

baronfel commented 2 years ago

I agree. A few other people have spoken up as well. We should remove the implicit mode and provide a helpful error if the user doesn't specify a ChangelogFile. That way there's no confusion about if something is happening or not.

MangelMaxime commented 6 months ago

Looking at the code it seems like there are already severals checks done for the file existence:

https://github.com/ionide/KeepAChangelog/blob/7ad0461e29bc3f22aafe971cdb11d61451198531/src/Ionide.KeepAChangelog.Tasks/buildMultiTargeting/Ionide.KeepAChangelog.Tasks.targets#L24-L28

https://github.com/ionide/KeepAChangelog/blob/7ad0461e29bc3f22aafe971cdb11d61451198531/src/Ionide.KeepAChangelog.Tasks/Library.fs#L49-L54

I guess they are currently only generating warnings and we should hard fail in these cases. What would be the correct way of failing?