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

New object parser, 100% test coverage, Multiline and sub-items. #37

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

Mulugruntz
Copy link

This is a follow-up of #32

Name `unlink` should only be used for File System.
Signed-off-by: Samuel Giffard <[email protected]>
The behaviour is the same, except for a few cases with `semantic_version` that didn't make sense. I consider it an improvement.
Another change is a smarter parsing of `release_date`.

Signed-off-by: Samuel Giffard <[email protected]>
It now properly keeps the empty lines and the case on `release_date`.
The `semantic_version` bug is also fix (improved consistency).

Signed-off-by: Samuel Giffard <[email protected]>
Signed-off-by: Samuel Giffard <[email protected]>
Note there is a weird quirk that uses `*` instead of `-` for the uncategorized bullet points. I kept this behaviour. But it could be fixed later (easy).

Signed-off-by: Samuel Giffard <[email protected]>
The order in which the changes appear is now deterministic. There were some minor inconsistencies before.

Signed-off-by: Samuel Giffard <[email protected]>
Signed-off-by: Samuel Giffard <[email protected]>
Signed-off-by: Samuel Giffard <[email protected]>
Signed-off-by: Samuel Giffard <[email protected]>
I split them into 2 commits, to have git give a correct diff (because of file renaming).

Signed-off-by: Samuel Giffard <[email protected]>
Multiline: Items can have several lines.

Sub-items: Items can contain other items.
Signed-off-by: Samuel Giffard <[email protected]>
Signed-off-by: Samuel Giffard <[email protected]>
Added pragma no cover to skip Version-specific lines.
GitHub Action added on Pull Request.

Signed-off-by: Samuel Giffard <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Jan 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Mulugruntz
Copy link
Author

@Colin-b Okay, now it should work.
I added the trigger on PR, which was missing.
3f07aec#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88R3

There's a need for approval to run the action, though?
Have a good evening!

@Colin-b
Copy link
Owner

Colin-b commented Feb 5, 2022

Thanks a lot. I just went through it quickly and this PR introduces a lot of changes and new features in one go.

While I am glad, I can't realistically assess the impact this way. It's my fault for not clarifying this in the contributing documentation, my bad.

Pull requests should be atomic. ie: contains a single change. If you would like to address more than the issue with the multi-line, I am all for it obviously. But it should be covered in an other pull request (and so on).

As for the changes in the test suite, they should be avoided as much as possible, removing test cases should not make sense, unless some features are actually removed. If a PR intent to fix a bug, it should come up with the appropriate additional test cases, but not impact the existing one. (or you can come up with a PR dedicated to rewriting the test suite) That way I can now for sure that previous behavior was not impacted.

I know this is a lot to ask, but could you split up this huge PR into multiple one? Each one focusing on one modification/fix/addition ? As you seemed to have split your commits this way, cherry picking on new branches should allow you to do it without too much hassle hopefully :/

@Mulugruntz
Copy link
Author

Hi @Colin-b,

Sorry for the long wait and thank you for your response.
About the "removing test cases should not make sense" you're mentioning, I don't think I removed any of the existing tests.
For example, the ones from test_changelog.py were moved to test_changelog_generic.py and not removed.
At some point in the development, I had a test file that I deemed too long, and that's why I split them into several files.

We now have 112 tests and 100% coverage ( https://github.com/Colin-b/keepachangelog/runs/5078766394?check_suite_focus=true#step:5:1 ), instead of 40 tests, 100% coverage ( https://github.com/Colin-b/keepachangelog/runs/4998576642?check_suite_focus=true#step:5:1 ).

I was also under the impression that this would be on the way to a new major version (current is 2.0.0.dev2), therefore, it would be acceptable to have breaking changes (for example, "release_date": "august 28, 2019" --> "release_date": "August 28, 2019" (capitalized)).

All the tests show how it currently behaves and give guarantee to users about this.

And of course, the CHANGELOG.md gives a summary about all the changes :-). (and I'm sure you'll appreciate that it is making use of the changes (sub-items, multine) ^^)

2.0.0.dev3 - 2022-01-30

Added

  • The parsing engine is now Object-oriented and more flexible.
  • Three new features:
    • Multiline: Items ( <-- such as this one) can now
      be written on several lines.
    • Sub-items: Items can now have sub-items.
      • Of any (reasonable) depth.
    • To list: Using to_list instead of to_dict makes it possible to sort it easily.

Changed

  • Release dates are parsed (several formats are supported).
    When to_dict(), a unique format is chosen.
  • When to_raw_dict(), release dates are kept "as-is"
    (and .lower() is no longer applied)

Fixed

  • Change ordering is now deterministic
  • When to_raw_dict(), empty lines are preserved.
  • When semantic version exists, it is produced. Before, if
    depended on some bugs (it was produced sometimes and not
    other times, such as when the change was empty or if there
    was only the link, something like that...).
    This is now more consistent.

However, I understand that, even though the commits are well split, such a big PR can feel overwhelming. So, if you'd like, we can have a ~2h video call in a weekend and I'll do my best to help you during the review of this PR (and I speak French :-) ). Moreover, I'm back to Europe, so I'm guessing we're now in the same timezone :-).

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