Skip to content
This repository has been archived by the owner on Apr 15, 2023. It is now read-only.

Add support for {date} replacement #33

Merged
merged 9 commits into from
Feb 22, 2021
Merged

Add support for {date} replacement #33

merged 9 commits into from
Feb 22, 2021

Conversation

BelKed
Copy link
Contributor

@BelKed BelKed commented Feb 14, 2021

Resolves #30.

  • Added date replacement
  • Better formatting in actions.yml + Added option for date format
  • Updated readme.md

@fregante
Copy link
Owner

fregante commented Feb 14, 2021

There are a lot of unrelated changes (the action.yml formatting isn’t “better” and is unrelated) and the file in distribution should not be modified manually. The source is /index.js. Please ensure that exclusively changes related to dates are included in this PR.

@fregante fregante marked this pull request as draft February 14, 2021 21:05
@BelKed
Copy link
Contributor Author

BelKed commented Feb 15, 2021

  • Oh, I will move edits to /index.js. I wondered why index.js in distribution had so many lines – I think, that was file, I should edit.
    • Also, shouldn't be it in .gitignore and builded and committed by Github Action on push/pull request?
  • And I have to fix bug, where longer or shorter dates caused bad date, hash and title of commit in description.

  • There should be updated documentation with building/contributing/developing...

@BelKed BelKed marked this pull request as ready for review February 15, 2021 08:32
action.yml Show resolved Hide resolved
generate-release-notes.js Outdated Show resolved Hide resolved
generate-release-notes.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@BelKed BelKed requested a review from fregante February 15, 2021 15:28
@fregante
Copy link
Owner

fregante commented Feb 15, 2021

shouldn't be it in .gitignore and builded and committed by Github Action

Unfortunately this doesn’t work with GitHub Actions. They want a pre-built file committed to the repo.

Also no need to force push. Just add commits normally.

generate-release-notes.js Outdated Show resolved Hide resolved
Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Can you also add some tests to ensure it works?

index.js Outdated Show resolved Hide resolved
@BelKed
Copy link
Contributor Author

BelKed commented Feb 15, 2021

Can you also add some tests to ensure it works?

And how?
I tested it in on my private repo and date option works...

@fregante
Copy link
Owner

There’s a file that ends with .test.js. Look at how the other features are tested a create similar tests for this new option. It should test for both a keyword and some custom formats

@BelKed
Copy link
Contributor Author

BelKed commented Feb 15, 2021

I added date test, but after I run it it except a error on generates changelog with custom date presets. It's function with test custom date format. I don't know what is wrong with it.

Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Perfect, thanks! I’ll leave it here a day or 2 and then merge it if no one complains

@BelKed
Copy link
Contributor Author

BelKed commented Feb 16, 2021

I added more date formats, especially for the date without leading zero and also for testing in generate-release-notes.test.js... It's using dateformat.

@fregante
Copy link
Owner

Please undo, git date formats are good enough.

@fregante
Copy link
Owner

Also no need to force push. Just add commits normally.

Copy link
Collaborator

@notlmn notlmn left a comment

Choose a reason for hiding this comment

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

Apart from the minor nits, looks good to me.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@BelKed
Copy link
Contributor Author

BelKed commented Feb 19, 2021

I remade whole test, because custom date format check is failing. It was failing because generateReleaseNotes function does not run git log with custom date parameters...

@fregante
Copy link
Owner

Not a bad idea, I dislike mocks for this exact reason, they give a false sense of security.

@BelKed
Copy link
Contributor Author

BelKed commented Feb 21, 2021

Could you merge this pull request now, so I can open another, that fixes another issue?

@fregante fregante changed the title Add date replacement Add support for {date} replacement Feb 22, 2021
@fregante fregante merged commit f047902 into fregante:main Feb 22, 2021
@fregante fregante added the enhancement New feature or request label Feb 22, 2021
@fregante
Copy link
Owner

Thank you @BelKed!

@fregante
Copy link
Owner

Unfortunately the commit titles aren’t being excluded from the replacement. The commit here included {date} and was replaced with the actual date.

7C1EE901-BF0F-4106-8BEC-C2F460768459

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date placeholder/replacement
3 participants