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

Follow markdownlint rules #28

Closed
Ndpnt opened this issue Dec 14, 2022 · 14 comments
Closed

Follow markdownlint rules #28

Ndpnt opened this issue Dec 14, 2022 · 14 comments

Comments

@Ndpnt
Copy link

Ndpnt commented Dec 14, 2022

Hi,

Thanks for this useful tool to generate changelog.

Could you modify the generated markdown to follow the rules defined by markdownlint to enforce consistency in Markdown files.

Currently the two rules that are not respected are:

@MattiSG
Copy link

MattiSG commented Jan 16, 2023

@oscarotero this issue seems to encompass #27 and #22. We @OpenTermsArchive are considering implementing it. Would you welcome such a PR? 🙂 I see you already have two pending PRs and we are wondering what's your level of maintenance of this open-source library, to understand if we should contribute (preferred) or fork.

@oscarotero
Copy link
Owner

Hi, @Ndpnt and @MattiSG

Thanks for your contribution. Yes, this project is actively maintained.

I can see the benefits of following the markdownlint rules, but I'd like have it as an optional feature. There are many people that prefer the original compact version. What do you think about something like this?:

const string = changelog.toString({ markdownlint: true });

@Ndpnt
Copy link
Author

Ndpnt commented Jan 17, 2023

Hi @oscarotero and thanks for the quick response.

It's a good idea to have an option to pass to the toString method but I think it might be better to have a linted output by default and pass an option for those who prefer the compact version:

const string = changelog.toString({ compact: true });

This way, by default it enforces standards and consistency in changelogs files spread across repositories.

@oscarotero
Copy link
Owner

The problem with this is it will change all existing markdowns and force to people already using this library to configure it to back to the expected compact version.
It would be a breaking change, so maybe in the future 3.x version this can be enabled by default, but for now I think it would be nice to have this option disabled to release the version 2.2.0.

@Ndpnt
Copy link
Author

Ndpnt commented Jan 17, 2023

Ok, I understand the problem. So, what about releasing now a major version with this improvement?

@oscarotero
Copy link
Owner

I'd like to have it also in 2.x version, but as a configurable option.

If you don't want to configure the library, another option is a static property, like Changelog.format = "markdownlint", so it's set once and applied always.

@Ndpnt
Copy link
Author

Ndpnt commented Jan 17, 2023

I was just in favor of enforcing standards by default but It's OK to configure the library.
I think it's best to have both options: a toString method option and also a static property.

@oscarotero
Copy link
Owner

Ok, thanks for understanding!

For clarifying, are you going to modify the PR to include these options?

@Ndpnt
Copy link
Author

Ndpnt commented Jan 18, 2023

I'm not really familiar with your codebase and I think it will be much easier and quicker for you to make the changes and to create the associated documentation, so I'd really appreciate it if you could handle the changes, if not I will.

@oscarotero
Copy link
Owner

Okay, no problem. I'll work on that

@Ndpnt
Copy link
Author

Ndpnt commented Jan 18, 2023

Thanks! 🙏

@oscarotero
Copy link
Owner

Done. I've released 2.2.0 with the format option.

@janhalama
Copy link

Done. I've released 2.2.0 with the format option.

Thanks for the release, can you also publish it on npm registry. It is not published yet: https://www.npmjs.com/package/keep-a-changelog

@oscarotero
Copy link
Owner

Ops, sorry. I thought it was published automatically.
Done!

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 a pull request may close this issue.

4 participants