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.

Sign up for GitHub

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

Jump to bottom

Make max length of body lines configurable #96

Merged
merged 6 commits into from
Feb 21, 2022
Merged

Make max length of body lines configurable #96

merged 6 commits into from
Feb 21, 2022

Conversation

Fryuni
Copy link
Contributor

@Fryuni Fryuni commented Feb 10, 2022

The limit of 72 characters is great for strict terminal use and some
GUIs, but for teams that use only UIs such as GitHub Web it is an
unnecessary and harsh restriction.
This PR adds a max-body-line-length option to make this
configurable.

Signed-off-by: Luiz Ferraz [email protected]

@mristin
Copy link
Owner

mristin commented Feb 11, 2022

Hi @Fryuni,
Thanks a lot for your contribution! I'll try to review the pull request over the weekend; worst case next week.

@Fryuni Fryuni mentioned this pull request Feb 11, 2022
Merged
mristin
mristin requested changes Feb 12, 2022
View reviewed changes
Copy link
Owner

@mristin mristin left a comment

Choose a reason for hiding this comment

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

Thanks @Fryuni! LGTM, please see a minor suggestion regarding the text in the readme.

README.md Outdated Show resolved Hide resolved
@mristin
Copy link
Owner

mristin commented Feb 12, 2022

@Fryuni Let us first merge in #97 and then have another look at this PR after you rebase it to master branch?

@mristin
Copy link
Owner

mristin commented Feb 12, 2022

@Fryuni While you are at it, perhaps you want to add the option max-subject-line-length for completeness?

@Fryuni Fryuni force-pushed the croct-tech/allow-configurable-max-body-line-length branch from 7a6d58c to 82d93f2 Compare February 14, 2022 03:02
@Fryuni
Copy link
Contributor Author

Fryuni commented Feb 14, 2022

@mristin I was off during the weekend. Rebased and implemented the configuration option for the subject line
WDYT?

@mristin
Copy link
Owner

mristin commented Feb 14, 2022

@Fryuni thanks, I'll try to review it tonight (CET).

I've just merged #97. Could you please rebase? Please mind the change #100 which was necessary for tests to pass in #97; the tests in this PR probably also have to be changed.

Fryuni and others added 4 commits February 14, 2022 16:45
@Fryuni
Make max length of body lines configurable
30a702b 
The limit of 72 characters is great for strict terminal use and some
GUIs, but for teams that use only UIs such as GitHub Web it is an
unnecessary restriction.
This commit adds a `max-body-line-length` option to make this
configurable

Signed-off-by: Luiz Ferraz <[email protected]>
@Fryuni
Add the new option to the action descriptor file
c6744ae 
Signed-off-by: Luiz Ferraz <[email protected]>
@Fryuni @mristin
Update README.md
1cead0a 
Co-authored-by: Marko Ristin <[email protected]>
@Fryuni
Make max length of subject configurable
445d9e3 
Signed-off-by: Luiz Ferraz <[email protected]>
@mristin
Copy link
Owner

mristin commented Feb 16, 2022

Hi @Fryuni ,
Have you perhaps missed the conflicts with the main branch? As soon as you re-base, we can merge in this PR.

@mristin
Copy link
Owner

mristin commented Feb 19, 2022

Hi @Fryuni ,
If you lack time, I could pick up this PR and finish it. What do you think?

I'll have some time from next Sunday on.

@Fryuni
Copy link
Contributor Author

Fryuni commented Feb 19, 2022

Hey @mristin, I just didn't have the time during the week, but I'll do it today or tomorrow :)

@Fryuni
Update tests
492d854 
Signed-off-by: Luiz Ferraz <[email protected]>
@Fryuni Fryuni force-pushed the croct-tech/allow-configurable-max-body-line-length branch from 82d93f2 to 492d854 Compare February 19, 2022 15:32
@Fryuni
Copy link
Contributor Author

Fryuni commented Feb 19, 2022

Turns out I only needed to update the inputs in the tests, the major changes that got to master had no conflict with mine

mristin
mristin requested changes Feb 20, 2022
View reviewed changes
Copy link
Owner

@mristin mristin left a comment

Choose a reason for hiding this comment

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

LGTM! Please see a minor suggestion for the docs. Other than that, this PR is ready to be merged in.

Thanks!

README.md Show resolved Hide resolved
@Fryuni @mristin
Update README.md
001ef9d 
Co-authored-by: Marko Ristin <[email protected]>
mristin
mristin approved these changes Feb 21, 2022
View reviewed changes
Copy link
Owner

@mristin mristin left a comment

Choose a reason for hiding this comment

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

LGTM! Ready for merge?

@Fryuni
Copy link
Contributor Author

Fryuni commented Feb 21, 2022

Seems ready to me

@mristin mristin merged commit a4b4e2a into mristin:master Feb 21, 2022
@mristin
Copy link
Owner

mristin commented Feb 21, 2022

@Fryuni thanks again! I'm going to release a new minor version next week.

@Fryuni Fryuni deleted the croct-tech/allow-configurable-max-body-line-length branch February 22, 2022 19:32
mristin added a commit that referenced this pull request Feb 27, 2022
@mristin
Release 3.0.0
d363845 
Non-breaking changes:

* Make max length of lines configurable (#96)
* Allow skipping message body check (#97)

Breaking change:

* Discontinue local scripts (#98)
@mristin mristin mentioned this pull request Feb 27, 2022
Merged
mristin added a commit that referenced this pull request Feb 27, 2022
@mristin
Release 3.0.0 (#101)
40ed5f5 
Non-breaking changes:

* Make max length of lines configurable (#96)
* Allow skipping message body check (#97)

Breaking change:

* Discontinue local scripts (#98)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mristin mristin mristin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Fryuni @mristin