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

feat(main): add options to configure scope related behaviour #75

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

sruehl
Copy link
Contributor

@sruehl sruehl commented Nov 16, 2021

Description

commits without scopes are not picked up. This PR solves this.

#11
#73

How Has This Been Tested

added a testcase for strip commits with a feat: something commit style. Test successful no regression.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Code follows the code style of this project.
  • Changes follow the CONTRIBUTING guidelines.
  • Update necessary documentation accordingly.
  • Lint and tests pass locally with the changes.
  • Check issues and pull requests first. You don't want to duplicate effort.

Other information

scope = re.findall(regex, head)[0]
if scope == '':
scope = 'all'
if scope.lower() == 'changelog' and regex == r'^docs(?:[(](.+?)[)])?':
Copy link

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

@gitmagic-app
Copy link

gitmagic-app bot commented Nov 16, 2021

Everything looks good 👍

@sruehl
Copy link
Contributor Author

sruehl commented Nov 16, 2021

btw: @BobAnkh your gitmagic-app demands the optional scope too...

@BobAnkh
Copy link
Owner

BobAnkh commented Nov 16, 2021

btw: @BobAnkh your gitmagic-app demands the optional scope too...

😆 Just forget about the robot

I will take a look at your codes ASAP.

main.py Outdated
if re.match(regex, head):
scope = re.findall(regex, head)[0]
if scope == '':
scope = 'all'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BobAnkh look closely at this line: I just throw empty scopes in a 'all' scope here.
There are two solutions

  1. don't touch the scope here and handle it smart during rendering (so no subsection with all)
  2. Leave it like it is and accept that we have a sub-section with all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's better to leave it empty and don't open a sub section for them

Copy link
Owner

Choose a reason for hiding this comment

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

Is it a good idea to put this kinds of commits in a sub-section with * instead of all?

Maybe it's better to leave it empty and don't open a sub section for them

Can you explain it to me in more details? Do you mean this :

-
  - add xxx
  - remove xxx

which renders that:

    • add xxx
    • remove xxx

I suppose it could not pass the markdown lint check?

Maybe a catch-all subsection *? (Because single * character cannot be used in list, so I use *)

  • *
    • add xxx
    • remove xxx

Do you think it a good choice? If this is not a good idea, I suppose to just add them at the same level of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Im also not quite sure. An alternative to all could be general. Even better if we add a action configuration for the general section which defaults to general or all so then the user can decide under which section these items should occur.

What you showing are lists of lists and what I mean is rendering them directly als list:

e.g. now

  • all
    • some general commit
    • some general commit
  • Bugfixes
    • some fix commit
    • some fix commit
  • Features
    • some feat commit

an this would be as simple list:

  • some general commit
  • some general commit
  • Bugfixes
    • some fix commit
    • some fix commit
  • Features
    • some feat commit

But looking at these now it might look cleaner if we have this catch-em-all section:

  • General
    • some general commit
    • some general commit
  • Bugfixes
    • some fix commit
    • some fix commit
  • Features
    • some feat commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my bad the types a sections and the lists elements are the scopes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like that

  DEFAULT_SCOPE:
    description: "The scope under which unscoped commits can be found e.g. 'fix: some general fix'"
    required: false
    default: 'general'
  SUPPRESS_UNSCOPED:
    description: "Suppress the generation of release notes for un-scoped commits e.g. 'fix: some general fix'"
    required: false
    default: 'false'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure will do

Copy link
Owner

@BobAnkh BobAnkh Nov 16, 2021

Choose a reason for hiding this comment

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

Thank you for your help. You can just doing the job and ignore all the suggestions proposed by bots. I will help to fix.

I will remove these bots when I am free 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, I think the bots are fine... bit verbose but they do their job 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes pushed btw.

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2021

Thanks for opening this pull request!
Please check out the Contributing Guidelines.

@mergify mergify bot assigned sruehl Nov 16, 2021
@mergify mergify bot requested a review from BobAnkh November 16, 2021 15:37
@mergify mergify bot added the bug Something isn't working label Nov 16, 2021
@BobAnkh BobAnkh added the enhancement New feature or request label Nov 16, 2021
@BobAnkh BobAnkh added this to the v1.1 milestone Nov 16, 2021
Repository owner deleted a comment from gitmagic-app bot Nov 16, 2021
- DEFAULT_SCOPE: lets the user define a scope under which all un-scoped commits can be found (defaults to general).
- SUPPRESS_UNSCOPED: can be used to have a pre-bugfix-behaviour in which all unscoped commits will get ignored.
changelog = GithubChangelog(ACCESS_TOKEN, REPO_NAME, PATH, BRANCH, PULL_REQUEST, COMMIT_MESSAGE, COMMITTER)
changelog.get_data()

CHANGELOG = generate_changelog(changelog.read_releases(), part_name)
CHANGELOG = generate_changelog(changelog.read_releases(), part_name, DEFAULT_SCOPE, SUPPRESS_UNSCOPED == 'true')
Copy link

Choose a reason for hiding this comment

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

line too long (116 > 79 characters)

@@ -363,7 +376,7 @@ def generate_changelog(releases, part_name):
if description == '':
description = '*No description*'
release_info = f'''## [{title}]({url}) - {date}\n\n{description}\n\n'''
release_body = generate_release_body(release_commits, part_name)
release_body = generate_release_body(release_commits, part_name, default_scope, suppress_unscoped)
Copy link

Choose a reason for hiding this comment

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

line too long (106 > 79 characters)

'''
Generate CHANGELOG

Args:
releases: dict of release data
part_name (list): a list of part_name, e.g. feat:Feature
default_scope (str): scope which matches all un-scoped commits
suppress_unscoped (bool): flag which suppresses entries for un-scoped commits
Copy link

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

@@ -304,19 +315,21 @@ def generate_release_body(release_commits, part_name):
# TODO: add a new attribute to ignore some commits with another new function
for part in part_name:
regex, name = part.split(':')
sec = generate_section(release_commits, regex)
sec = generate_section(release_commits, regex, default_scope, suppress_unscoped)
Copy link

Choose a reason for hiding this comment

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

line too long (88 > 79 characters)

'''
Generate release body using part_name_dict and regex_list

Args:
release_commits (dict): commits of the release
part_name (list): a list of part_name, e.g. feat:Feature
default_scope (str): scope which matches all un-scoped commits
suppress_unscoped (bool): flag which suppresses entries for un-scoped commits
Copy link

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

'''
Bypass some commits

Args:
commits (list): list of commit(dict), whose keys are 'head', 'sha', 'url', 'pr_links'
type_regex (string): regex expression to match.
default_scope (str): scope which matches all un-scoped commits
suppress_unscoped (bool): flag which suppresses entries for un-scoped commits
Copy link

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

@@ -87,7 +87,7 @@ def set_env_from_file(file, args, prefix='INPUT'):
params = step['with']
break
option_params = [
'REPO_NAME', 'ACCESS_TOKEN', 'PATH', 'COMMIT_MESSAGE', 'TYPE', 'COMMITTER'
'REPO_NAME', 'ACCESS_TOKEN', 'PATH', 'COMMIT_MESSAGE', 'TYPE', 'COMMITTER', 'DEFAULT_SCOPE', 'SUPPRESS_UNSCOPED'
Copy link

Choose a reason for hiding this comment

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

line too long (120 > 79 characters)



@pytest.mark.parametrize("releases, part_name, changelog", case['test_generate_changelog'])
def generate_changelog(releases, part_name, changelog):
for release in releases:
releases[release]['created_at'] = datetime.datetime(2000, 1, 2, 3, 4, 5)
assert changelog == main.generate_changelog(releases, part_name)
assert changelog == main.generate_changelog(releases, part_name, "general", False)
Copy link

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)



@pytest.mark.parametrize("release_commits, part_name, release_body", case['test_generate_release_body'])
def test_generate_release_body(release_commits, part_name, release_body):
assert release_body == main.generate_release_body(release_commits, part_name)
assert release_body == main.generate_release_body(release_commits, part_name, "general", False)
Copy link

Choose a reason for hiding this comment

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

line too long (99 > 79 characters)



@pytest.mark.parametrize("release_commits, type_regex, sec", case['test_generate_section'])
def test_generate_section(release_commits, type_regex, sec):
assert sec == main.generate_section(release_commits, type_regex)
assert sec == main.generate_section(release_commits, type_regex, "general", False)
Copy link

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)

@gitmagic-app
Copy link

gitmagic-app bot commented Nov 16, 2021

@sruehl - Your pull request does not follow our contribution guidelines. Please review the following issues and update.

<header> of commit message needs to be less than 51 characters long

If you have any questions, please refer to our Contributing Guidelines or ask us here.

Thanks

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2021

@sruehl
Contribution Message Convention Tests failed with GitMagic.
Please check details and Contributing Guidelines for more information.
If you don't want to correct it yourself, just tell the maintainers. They will do it when merging.

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #75 (38c7fcd) into master (920f902) will increase coverage by 0.64%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   31.15%   31.80%   +0.64%     
==========================================
  Files           2        2              
  Lines         276      283       +7     
==========================================
+ Hits           86       90       +4     
- Misses        190      193       +3     
Impacted Files Coverage Δ
main.py 24.60% <75.00%> (+0.92%) ⬆️
tests/test_main.py 90.32% <75.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 920f902...38c7fcd. Read the comment docs.

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2021

@sruehl
Markdown Files Check failed with markdownlint.
Please check details or Contributing Guidelines for more information.

@BobAnkh BobAnkh changed the title fix(main): scopeless commits will be picked up now feat(main): add options to configure scope related behaviour Nov 16, 2021
@BobAnkh BobAnkh merged commit d9d66d2 into BobAnkh:master Nov 16, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 16, 2021

Awesome work, congrats on your first merged pull request!

@BobAnkh
Copy link
Owner

BobAnkh commented Nov 16, 2021

A new version will be released if everything is fine with the changelog generated by the new action.

@BobAnkh
Copy link
Owner

BobAnkh commented Nov 16, 2021

Really appreciate your help!

@sruehl sruehl deleted the fix/include_scopeless_commits branch November 16, 2021 17:58
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue or pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request Type: CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Unscoped commits should use all scope [enhancement] add an option to choose whether to use <scope>
2 participants