Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some new configuration options and other small improvements #1372

Draft
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

selfdocumentingcode
Copy link
Contributor

@selfdocumentingcode selfdocumentingcode commented Sep 14, 2024

Hello!

I added a couple of new features that I found useful for my use case and also made some refactoring along the way.
The PR is rather messy and I haven't cleaned up yet since I wasn't sure if you will consider merging this or not.
If that turns out to be the case, I can fix it up a bit as well as update the documentation.

I managed to keep backwards compatibility (I think) and keep the tests running for the most part. I didn't manage to run the Gitea tests, even on the unchanged develop branch. I got a token and everything, but I don't know. Would appreciate some help here if this PR were to be merged.

Anyway, the changes I made are as follows:
Features:

  • Add commit_template property to use for "fake" PRs in HYBRID and COMMIT modes.
    Did my best to keep it backwards compatible, but there might be some issues with cached configurations. In the local tests, the way the configuration is loaded and prioritized works a bit differently than it does in "prod", so it's hard to say if it's 100% backwards compatible
  • Add optional categorized_include_empty_content boolean property to make categorized_output category entries include the empty_content string as an array item when the array would be empty i.e. no PRs.
  • Add optional mode property for category which makes it possible to define a category that only applies to either real PRs or fake PRs.
    By default it's in HYBRID mode which makes it apply to both types of PRs just like before.

Refactoring:

  • Extracted more functions out of the large buildChangelog function as well as making the new and existing ones either deal with building the placeholder maps (which I renamed to "template contexts" [inspired by Scriban/Liquid] since I thought too many things were called placeholder something) or deal with "rendering" i.e. substituting tokens in the template strings.

Misc. changes:

  • I had to add plugin:@typescript-eslint/recommended in order to fix an issue where the built-in Record<T,U> type was not being recognized. I have no idea how adding that plugin fixed that, but it did. I can, of course, revert it in this PR if it's unwanted.

test:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
steps:
# the below actions use the local state of the action. please replace `./` with `mikepenz/release-changelog-builder-action@{latest-release}`
# Showcases how to use the action without a prior checkout
# Since 3.2.0 the configuration can be provided within the `yml` file
- name: "Configuration without Checkout"
- name: 'Configuration without Checkout'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please revert the changes made here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. That's how prettier thought to format the file.
I added an override in for prettier to use double quotes in .yml and re-formatted it.
It's not exactly reverted, since some quotes were single quotes from before, but it should be more consistent now; I hope you agree with the change.

node_modules/
__tests__/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and I will revert it, but I am experience issues when eslint is active for the files in __test__ directory.
For example:
image

I am using VSCode with the latest version of ESLint.
Do you happen the know what the issue is?

Comment on lines -4 to -7
max_tags_to_fetch: number
max_pull_requests: number
max_back_track_time_days: number
exclude_merge_branches: string[]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the removal of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are already inherited from PullConfiguration.

@mikepenz
Copy link
Owner

Thank you very much @selfdocumentingcode for the proposal and for the PR.

I scanned through the changes on a high level, to get a better understanding of the changes.
And I really like the proposed enhancements.

That said, would it please be possible to break the PR into smaller PRs please. E.g. have the refactoring in its own PR.
And then also the individual features in their own PR.

Additionally, I'd appreciate if you could please revert the unrelated changes, like the updates of the dependencies
Equally, I do see more changes which should please be reverted:
Screenshot 2024-09-15 at 11 12 38

@selfdocumentingcode
Copy link
Contributor Author

@mikepenz Sounds good! I will start setting up the refactoring PR.

@selfdocumentingcode
Copy link
Contributor Author

@mikepenz I created #1373 with the refactoring changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants