Skip to content

[Feature Request] Line length warning #191

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

Closed
Lee-W opened this issue May 23, 2020 · 16 comments · Fixed by #1076
Closed

[Feature Request] Line length warning #191

Lee-W opened this issue May 23, 2020 · 16 comments · Fixed by #1076
Assignees

Comments

@Lee-W
Copy link
Member

Lee-W commented May 23, 2020

Description

It's recommended to have only 72 characters. We could add a warning or a hint if the generated title might exceed 72.

Possible Solution

Add a line hint like vim

image

Additional context

Related Issue

@woile
Copy link
Member

woile commented May 23, 2020

You mean for the commit subject? Where would we add this?

Remember that this is not a problem for the changelog. I'm not sure why the line length was chosen to be 72.

@Lee-W
Copy link
Member Author

Lee-W commented May 23, 2020

The first line of our generated commit. e.g., feat(new): awesome feature.
I'm thinking of make the functions in commitizen/utils.py and this funtionality something like commitizen-plugins. But it's still a rough idea.

I guess the number is from here

@woile
Copy link
Member

woile commented Jun 2, 2020

IMO we should add it as a config parameter (max_subject_length), disabled by default, or set to 0 which would mean: anything.

I like the plugins idea, not sure how to hook everything haha but it's interesting

@Lee-W
Copy link
Member Author

Lee-W commented Jun 2, 2020

This could be my next target after #128 haha. Also, if my memory serves me right, you've mentioned implementing changelog-hooks (like email-hooks or slack-hooks). Do you have any idea how we should group them. Add them into to this repo as extra deps? Or, create another repo like commitizen-changelog-hooks?

@woile
Copy link
Member

woile commented Jun 2, 2020

I was thinking more like separate packages, so people can choose which one to install:

pip install cz-slack-hook cz-email-hook

and then in your cz.toml

hooks = [
  "slack-hook",
  "email-hook"
] 

@jtpavlock
Copy link
Contributor

jtpavlock commented Jul 23, 2020

I agree with the 72 character default for the commit title, as that's the max github displays in the commit history.

@woile woile added type: feature A new enhacement proposal and removed enhancement labels Jul 24, 2020
@Lee-W
Copy link
Member Author

Lee-W commented Sep 25, 2021

Here's a way to implement similar functionality #331 (comment). Not yet tried this functionality on cz-cli. might need some investigation

@changchaishi
Copy link

Hi there!
I found a feasible solution for line length warning. It is done by questionary's text validator, which will show a red color background warning at the bottom-left corner of the console.

the sample code is:

import questionary
from questionary import Validator, ValidationError

class NameValidator(Validator):
    def validate(self, document):
        text_len = len(document.text)
        if True:
            raise ValidationError(
                message=f"({text_len}/72)",
                cursor_position=text_len,
            )

questionary.text("Hi, type anything below:\n", validate=NameValidator).ask()

and result is:
questionary_validator

This approach considered all text validation results are always False, so that it can keep up and update the warning text at below. Another approach is to stop the input cursor when text reaches maximum length.

@Lee-W Lee-W removed the pycontw2021 label Sep 27, 2021
@Lee-W
Copy link
Member Author

Lee-W commented Sep 28, 2021

Thanks for sharing! This is really helpful. I'm thinking of making those two behavior confiugrable

@woile
Copy link
Member

woile commented Sep 28, 2021

That's super cool finding!

@jubenitezg
Copy link

Hello everyone!
I liked @iknowright 's solution, unfortunately from the Questionary docs - "A user can not submit an answer if it doesn’t pass the validation.", however, his approach can be modified to show to the user when the 72 character limit is exceeded.

Result:

import questionary
from questionary import Validator, ValidationError

class NameValidator(Validator):
    def validate(self, document):
        text_len = len(document.text)
        if text_len > 72:
            raise ValidationError(
                message=f"({text_len}/72)",
                cursor_position=text_len,
            )

questionary.text("Hi, type anything below:\n", validate=NameValidator).ask()

length-limit

Any thoughts on this?

@Lee-W
Copy link
Member Author

Lee-W commented Mar 13, 2022

I'm good with it 👍

@kevin1kevin1k
Copy link
Contributor

I'm trying to work on this issue, and found one interesting bottleneck:
If we want to validate the commit message from prefix to subject, i.e., prefix(scope): subject,
then the validating function or class would need to know about the previously-input prefix and scope, which I found difficult to do now via something like the following.

"type": "input",
"name": "subject",
# added validator cannot access the results from other questions
"validate": SomeValidator,
"filter": parse_subject,
"message": (
    "Write a short and imperative summary of the code changes: (lower case and no period)\n"
),

Therefore, before questionary supports the desired access, one workaround is to validate the length in, for example, ConventionalCommitsCz.message(), where the results are available in the answers dict.
There are some drawbacks:

  • The validation cannot be performed on the fly as desired
  • The exception is raised before git.commit, so we cannot use the --retry feature.

Please refer to #557.

Any ideas or better solutions are welcome!

@rkr87
Copy link

rkr87 commented Aug 16, 2022

I totally understand it's not desirable but I don't think there's any particular issue in this case, but couldn't this be achieved with a global variable update on type and scope update? I'm new to Commitizen and Python in general so take any of my suggestions with a pinch of salt, but wouldn't doing something like below work?

MESSAGE_LENGTH = 0

def parse_type(text):
    global MESSAGE_LENGTH
    MESSAGE_LENGTH += len(text)

def parse_scope(text):
    if not text:
        return ""
    global MESSAGE_LENGTH
    scope = text.strip().split()
    
    if len(scope) == 1:
        MESSAGE_LENGTH += len(scope[0])
        return scope[0]
    text = "-".join(scope)
    MESSAGE_LENGTH += len(text)
    return text

Then use MESSAGE_LENGTH in your SomeValidator?

@jtpavlock
Copy link
Contributor

Would it be possible to also check the configured line length with cz check in case you commit without using the prompt?

@Lee-W
Copy link
Member Author

Lee-W commented Sep 10, 2022

maybe we could make the length an option 🤔 @kevin1kevin1k will you be interested in it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants