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

Fix: correctly get whole body of commit messages #6

Merged
merged 4 commits into from
May 14, 2020

Conversation

JPeer264
Copy link
Contributor

When commits had more than one line it got separated as it was an own commit when type: 'text'.

Feat: my description

my body

got parsed to

["Feat: my description", "", "got parsed to"]

this fix makes it to

["Feat: my description\n\ngot parsed to"]

@coveralls
Copy link

coveralls commented May 12, 2020

Coverage Status

Coverage increased (+0.3%) to 85.0% when pulling 9aeb24a on JPeer264:fix/text-format into 578040c on aichbauer:master.

Copy link
Owner

@aichbauer aichbauer left a comment

Choose a reason for hiding this comment

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

Is this a complete repo? Do we need that much commits in it? I guess this will confuse us in the future. I haven't touched that repo in a while and I would have to take a look at how the fixtures were made... but this seems a bit too much, what so you think?

@JPeer264
Copy link
Contributor Author

This is a copy of the other repo, but instead of 10 commits (as in the original repo) it is just reduced to 4 commits. Idk why there are so many objects, but we won't touch the plain git files anyway.

@JPeer264
Copy link
Contributor Author

Maybe because I copied it and removed the last 6 commits. I could add a new repo to make it less files? But that depends on you, if you want me to do that :)

@aichbauer
Copy link
Owner

Do we need that many commits? We are only testing if the body message is copied correctly right? So we could simply add one or two commits to the existing repo fixture right?

@JPeer264
Copy link
Contributor Author

I wanted to cover 4 cases.

  1. one liner
  2. just one break after each other
  3. multiple line breaks
  4. a one liner after all line breaks (to check it won't break anything after)

@JPeer264
Copy link
Contributor Author

better to have more tests than too less :D

@@ -0,0 +1,28 @@
15be93c31ad87c9ced03ba0b60fc2fb55c977c5c 1fd8a8cf346d4d4c671aa343b2e93ad01f92df3c JPeer264 <jan.oster94@gmail.com> 1589302305 +0200 rebase -i (start): checkout 1fd8a8cf346d4d4c671aa343b2e93ad01f92df3c
Copy link
Owner

Choose a reason for hiding this comment

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

what about this file? This does not exist in the current repo right?

@aichbauer
Copy link
Owner

Yep better safe than sorry...

I just think that this fixture will bloat this repo, because it is currently really hard to get an overview of what we need, and now we have two of them. And I do not quite think that you just copied the other repo. Can you tell me exactly what you did? We should probably add the commits to the old repo, since we already make all the tests with the other one.

@aichbauer
Copy link
Owner

So I digged a bit deeper into it... We only keep stuff that is needed to get the commits, we do not keep the whole repo in the fixtures, because this will bloat this package repo.

We do not need any .js files, we do not need the logs, etc...

Just add the commits to the old repo or use the new one, but remove all unnecessary stuff, just take a look at the current repo fixture to get an idea of what is really needed to test the commits.

@JPeer264
Copy link
Contributor Author

Ya I will check what is not needed. I will not add the commits to the existing repo, as it will fail the existing test cases.

@JPeer264
Copy link
Contributor Author

i removed now all unnecessary files

@aichbauer aichbauer merged commit 4b87074 into aichbauer:master May 14, 2020
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