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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): fix parsing commit date with different time zone #315

Merged

Conversation

ahrbil
Copy link
Contributor

@ahrbil ahrbil commented Aug 24, 2022

Description

Update regex for extracting commit date inside the get-commits-since-last-release.ts function, the regex has an issue related to parsing time zones. When executing git log with --format="%h %aI" option to get the oldest commit details, %aI formats the author date in ISO 8601 format, then the old regex /^"?([0-9a-f]+)\s([0-9\-T\:]*)"?$/ used to extract commit hash and date. The old regex can only parse dates with a time zone offset of - hours but in my case, my time zone has an offset of + hours. Hence, the commit date was always undefined leading to all commands that call Github API with $since variable to fail with GraphqlResponseError Variable $since of type GitTimestamp! was provided invalid value.

Thank you @ghiscoding for the fantastic work 馃檹.

Motivation and Context

After #272, I tried to use the --changelog-include-commits-client-login option but it always fails when calling GitHub API, after debugging turns out that the commit date was undefined because of miss-parsing of the ISO date time zones.

New Regex (regex101 editor)

/^"?([0-9a-f]+)\s([0-9\-|\+T\:]*)"?$/

should parse -+ time zones.

How Has This Been Tested?

All old tests pass and I don't know if this case needs a dedicated test.

Types of changes

  • Chore (change that has absolutely no effect on users)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ghiscoding
Copy link
Member

ghiscoding commented Aug 24, 2022

Thank you @ghiscoding for the fantastic work 馃檹.

Happy to help and thanks for using the lib 馃槈

oh thanks, I forgot where I took that regex from but that's totally a missed on my part. On the test side, could you just modify at least 1 of the current unit tests and change one of them to be a + instead of a - and it should be enough to test the new regex, for example modify the last unit test from -04:00 to your time zone diff

https://github.com/ghiscoding/lerna-lite/blob/d2990cc01763cb81e1a185771775309aec1e986e/packages/core/src/conventional-commits/__tests__/get-commits-since-last-release.spec.ts#L119-L130

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #315 (eb1f653) into main (f618ae9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #315   +/-   ##
=======================================
  Coverage   96.13%   96.13%           
=======================================
  Files         145      145           
  Lines        4237     4237           
  Branches      888      888           
=======================================
  Hits         4073     4073           
  Misses        164      164           
Impacted Files Coverage 螖
...entional-commits/get-commits-since-last-release.ts 97.23% <100.00%> (酶)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ghiscoding ghiscoding merged commit b120a90 into lerna-lite:main Aug 24, 2022
@ghiscoding
Copy link
Member

@ahrbil
Thanks again for the PR, I'll release a patch version later tonight after working hours. Cheers

@ahrbil
Copy link
Contributor Author

ahrbil commented Aug 24, 2022

Currently migrating away from Lerna to Lerna-lite with pnpm and turborepo, but I got blocked by this issue. Thank you @ghiscoding for your valuable time and work.

@ahrbil ahrbil deleted the fix/commit-date-with-different-time-zone branch August 24, 2022 19:25
@ghiscoding
Copy link
Member

ghiscoding commented Aug 24, 2022

Very welcome, I'm also concerned about some changes they've done in Lerna by the Nrwl team. It's really nice to see that Lerna is back but at the same time Nrwl is pushing so much for their Nx integration (they recently added Nx as a dependency of Lerna, but why??) which is not always great for users who don't want to use Nx (ie, users of TurboRepo like you). So in some ways, it pushes me to keep Lerna-Lite going 馃槈

@ghiscoding
Copy link
Member

published patch release v1.11.1 (lots of 1s there, must be a lucky release 馃槃)

@ahrbil
Copy link
Contributor Author

ahrbil commented Aug 25, 2022

I agree @ghiscoding, I think Lerna now will be more associated with Nx. The reason I'm migrating from Lerna is pnpm support and workspace: protocol, Nx team is working on that at the moment, and this will bring us to the second reason which is performance, Lerna used to be an all-in-one tool that does everything but nowadays we have dedicated tools that only specialize at one thing and does it very well, turborepo is much performant than Lerna exec and run, pnpm linking and resolving of workspace packages replaces Lerna add and bootstrap, sure there will be more configuration but this is something you don't deal with every day. The missing part for my migration was versioning and generating changelog from conventional commits, sure there is changesets but I think it is more dedicated to libraries, and also I like this command from Lerna and it's the most command I use, I was looking for alternatives but none of them were straightforward. I already heard about lerna-lite but back then doesn't have pnpm support, now the puzzle is solved with pnpm support landing.

@ahrbil
Copy link
Contributor Author

ahrbil commented Aug 25, 2022

published patch release v1.11.1 (lots of 1s there, must be a lucky release 馃槃)

You need to hear the fun part, it is also my first contribution to an open source project 馃槅.

@ghiscoding
Copy link
Member

ghiscoding commented Aug 25, 2022

Awesome, welcome to the Open Source world 馃槈 and feel free to reach out if it still doesn't work. I also agree with everything you said.

I already heard about lerna-lite but back then doesn't have pnpm support, now the puzzle is solved with pnpm support landing.

I added support for it about 3-4 months ago and I converted the project to pnpm workspace protocol about a month later. After a couple of small fixes around the feature, it's quite stable now, The biggest player which migrated to Lerna-Lite and is now using this feature is Jest, which I was quite thrilled to see them migrate just a couple weeks ago 馃樃 .. oh and talking about them, they just release v29.0.0 just an hour ago 馃殌

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.

None yet

2 participants