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

Use full unformatted subject message #1127

Merged
merged 4 commits into from
Mar 24, 2021
Merged

Use full unformatted subject message #1127

merged 4 commits into from
Mar 24, 2021

Conversation

denieler
Copy link
Contributor

We want to use danger js locally to check the format of conventional commits for the commit messages. Due to that, we need to have a full unformatted subject line. Similar issue is explained in #977

We want to use danger js locally to check the format of conventional commits for the commit messages. Due to that, we need to have a full unformatted subject line. Similar issue is explained in danger#977
@orta
Copy link
Member

orta commented Mar 19, 2021

This will probably need additional work - message is used to ensure it is a single line because it is serialized to JSON right after.

@denieler
Copy link
Contributor Author

denieler commented Mar 19, 2021

hey @orta 👋

This will probably need additional work - message is used to ensure it is a single line because it is serialized to JSON right after.

Tested the change and it looks like it works fine. Change %f to %s only changes formatting but left only a single line of the commit message (https://git-scm.com/docs/pretty-formats#Documentation/pretty-formats.txt-emsem, it takes only subject).

Screenshot 2021-03-19 at 12 13 10

I've tried to find a good place to add tests for this functionality but didn't find, also it looks a bit tricky to test git log output 😞 if you have any suggestions - please share.

And travis pr check is failing with some "Bad credentials message". Not sure that this change could cause it 😞

Screenshot 2021-03-19 at 12 46 44

Please help 😃

@denieler
Copy link
Contributor Author

hey @orta 👋 any news? can we merge this PR?

@orta
Copy link
Member

orta commented Mar 22, 2021

Remember I do this in my spare time.

Can you verify that having " in a commit message won't break with this change? You can ignore the red CI from travis, I still need to get that fixed

@denieler
Copy link
Contributor Author

@orta sure, checking...

@denieler
Copy link
Contributor Author

hey @orta! It appeared that previous implementation does not respect " or any other quotes 😃 After some research I found that it's fairly hard to do that via git log which was there before and requires some extra work, so found gitlog package - it provides what's necessary. Just had to shape it to the current usage 👍 Looks like now working fine. Please check.

P.S. Had to update typescript, because the new library using new types, which are supported in TS 3.8, but in Danger it was super outdated (3.1.1). And another change was because of that. But looks like it doesn't break anything.

@@ -70,7 +70,7 @@ export interface ExecutorOptions {
const isTests = typeof jest === "object"

interface ExitCodeContainer {
exitCode: number
exitCode?: number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because TS was updated it started to complain about

Screenshot 2021-03-22 at 17 51 36

So had to fix this type to fix it

@denieler
Copy link
Contributor Author

hey @orta ! Can you please review this PR? 🙇

@orta
Copy link
Member

orta commented Mar 24, 2021

I'm not a fan of adding new dependencies to Danger JS and I don't think this is worth it.

I'll take reverting back to 6a17936 and accept that 👍🏻

@denieler
Copy link
Contributor Author

@orta what do you think if I copy partially the part from the gitlog package into dangerjs code? Do you think this will be a better solution?

@orta
Copy link
Member

orta commented Mar 24, 2021

Maybe, kinda depends on how much code it is - given the goal is to import a string, switching " -> "▍" (some random char) before putting it into the JSON and flipping it on the other side would also solve the problem too.

@denieler
Copy link
Contributor Author

Maybe, kinda depends on how much code it is - given the goal is to import a string, switching " -> "▍" (some random char) before putting it into the JSON and flipping it on the other side would also solve the problem too.

There are few characters, which can affect JSON.parse, so I didn't really want to move this way. Ok, let me make a change and I will ping you one more time. If you don't like it - we can go with string.replace for a bunch of special characters

@denieler
Copy link
Contributor Author

@orta please take a look at the new change. I've not rolled back the typescript update, because it looks for me like a healthy dependency update for the library and it looks like not breaking anything

@orta
Copy link
Member

orta commented Mar 24, 2021

Looks great to me, let's get this shipped

@orta orta merged commit 69cd960 into danger:main Mar 24, 2021
@denieler denieler deleted the patch-1 branch March 24, 2021 13:39
@glensc
Copy link
Contributor

glensc commented May 3, 2022

the included gitlog is BSD-3-Clause licensed, meaning you must preserve any copyright notices, which is not done here.

read the license here:

the file in question:

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

3 participants