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: Add Rhondda Cynon Taff #358

Merged

Conversation

dp247
Copy link
Collaborator

@dp247 dp247 commented Oct 11, 2023

Hello, hello. It's me.

This closes #357 very nicely. Please let me know if I have broken anything... it has been a while hahaha.

@codecov-commenter
Copy link

Codecov Report

Merging #358 (5b7488a) into master (f8d77a1) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #358   +/-   ##
=======================================
  Coverage   83.44%   83.44%           
=======================================
  Files           3        3           
  Lines         151      151           
=======================================
  Hits          126      126           
  Misses         25       25           

@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

Looks like some commit lint errors to fix @dp247 😅

@dp247
Copy link
Collaborator Author

dp247 commented Oct 11, 2023

Looks like some commit lint errors to fix @dp247 😅

I can do that... once I figure out what that means 😅

@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

Looks like some commit lint errors to fix @dp247 😅

I can do that... once I figure out what that means 😅

Wish I knew too! Think @OliverCullimore added this nice bit of workflow

@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

Looks like a quality gate

@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

Try this PR #350

Which was green

@dp247 dp247 changed the title Rhondda Cynon Taff feat: Add Rhondda Cynon Taff Oct 11, 2023
@dp247
Copy link
Collaborator Author

dp247 commented Oct 11, 2023

Try this PR #350

Which was green

I thought it might be the feat: bit, but no dice

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Oct 11, 2023

@robbrad @dp247 it's checking the commit message rather than PR name sadly e.g. on #350 as @robbrad referenced the commit message contains a prefix of feat:

image

So essentially going forwards each commit should now have a prefix denoting it's type

The main prefixes I expect us to use are:

feat: and fix: for anything council-related
ci: for workflows
docs: for wiki/readme/contributing changes

This then enables https://commitizen-tools.github.io/commitizen/ to automatically bump the version for us based on our commit messages

Hope that all makes sense?


So @dp247 for your commits on this one they should've been prefixed with feat: as you're adding support for a new council e.g. "feat: Add supporting files for Rhondda Cynon Taff Council"

I'm not sure if we can edit commit messages to fix this? Otherwise it won't harm to merge this, it just won't generate a new version therefore won't be released and able to be used until another PR is merged that does trigger a release.

@OliverCullimore
Copy link
Collaborator

OliverCullimore commented Oct 11, 2023

@dp247 I've re-committed your support files commit with a feat: prefix so hopefully the PR merge should trigger a new version at least now even if the lint action is still failing on the other messages 😃

@OliverCullimore OliverCullimore merged commit f9e8f88 into robbrad:master Oct 11, 2023
4 of 5 checks passed
@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

Thanks @OliverCullimore this makes total sense. Sorry had a lot on recently and not paid close attention.

Could the same commit checks check for the files we need eg input.json etc?

@dp247
Copy link
Collaborator Author

dp247 commented Oct 11, 2023

Thank you! That's a nifty thing - would it be worth adding something to Contributing.md as a reminder?

@dp247 dp247 deleted the 357-request-to-add-rhondda-cynon-taff branch October 11, 2023 19:12
@OliverCullimore
Copy link
Collaborator

That seems to have done the trick so @dp247 that's your first changes release in the new way https://github.com/robbrad/UKBinCollectionData/releases/tag/0.13.0 😃

@robbrad no worries at all, it's a bit of a change to how we were doing things and a learning curve for me on the existence and implementation of it all. But hopefully, now it's set up, it will enable us to focus what little time we have on adding more councils and improvements rather than manually creating releases now.

@dp247 your commits have usefully also highlighted I missed updating the wiki action's commit message so I'll try and add a PR to fix that now 😄

And @robbrad I'm sure it should be possible to set something up to check for certain files but it would likely need some complex rules to handle adding a council vs fixing a small issue with an existing one, so maybe more something to revisit in the future?

@OliverCullimore
Copy link
Collaborator

Thank you! That's a nifty thing - would it be worth adding something to Contributing.md as a reminder?

.... I did add a line https://github.com/robbrad/UKBinCollectionData/blob/master/CONTRIBUTING.md#make-sure-commit-messages-follow-the-conventional-commits-convention but I'm guessing it might need better placement and rewording as even I just found it hard to find where I'd put it in there

@robbrad
Copy link
Owner

robbrad commented Oct 11, 2023

That seems to have done the trick so @dp247 that's your first changes release in the new way https://github.com/robbrad/UKBinCollectionData/releases/tag/0.13.0 😃

@robbrad no worries at all, it's a bit of a change to how we were doing things and a learning curve for me on the existence and implementation of it all. But hopefully, now it's set up, it will enable us to focus what little time we have on adding more councils and improvements rather than manually creating releases now.

@dp247 your commits have usefully also highlighted I missed updating the wiki action's commit message so I'll try and add a PR to fix that now 😄

And @robbrad I'm sure it should be possible to set something up to check for certain files but it would likely need some complex rules to handle adding a council vs fixing a small issue with an existing one, so maybe more something to revisit in the future?

Definitely parking lot.

This is awesome and I love that this project has supported a learning journey. It's a massive part of why I set it up all those years ago... to learn Python!!!

Kudos team!!

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.

Request to add Rhondda Cynon Taff
5 participants