Skip to content
This repository has been archived by the owner on May 3, 2020. It is now read-only.

kwalitee: addition of sentence dot check #95

Merged
merged 1 commit into from
Jul 18, 2016

Conversation

david-caro
Copy link
Contributor

No description provided.

@jirikuncar
Copy link
Contributor

@david-caro there are another valid ending: * Fixes foo bar. (closes #14) or * Fixes foo long\n message. (addresses #1).

@david-caro
Copy link
Contributor Author

Sorry, missed that one. the parenthesis there is something defined? must be on the same line?

@@ -141,17 +142,26 @@ def _check_bullets(lines, **kwargs):
"""
max_length = kwargs.get("max_length", 72)
labels = {l for l, _ in kwargs.get("commit_msg_labels", tuple())}
signatures = kwargs.get("signatures", tuple())

def _is_signature(line):
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To properly find the end of the bullet message, as the signatures don't have a dot they should be treated differently and never be considered part of the dotted message itself.

@david-caro david-caro force-pushed the add_dot_check branch 2 times, most recently from a9d529d to eef49ac Compare July 15, 2016 15:49
@david-caro
Copy link
Contributor Author

Sorry for the noise, for now I just expect the '()' to not be multiline, added tests for that case too.

@jirikuncar
Copy link
Contributor

jirikuncar commented Jul 15, 2016

IMHO we can do it with a regex /\.([ ]\{1,\}\([^)]\{1,\}\))?$/.

@david-caro
Copy link
Contributor Author

that does not work if the comment is like:

* bulleted thingie.
  (closes #123)

@jirikuncar
Copy link
Contributor

jirikuncar commented Jul 15, 2016

True. So we need something like /^[ *] (.*\.)?([ ]\{1,\}\([^)]\{1,\}\))?$/ for the last line of a paragraph?

@jirikuncar jirikuncar added this to the v1.0.0 milestone Jul 15, 2016
@jirikuncar jirikuncar changed the title Add dot check kwalitee: addition of sentence dot check Jul 15, 2016
@david-caro
Copy link
Contributor Author

at that point in the code, we have a line-by-line loop, so I would have to go reconstructing them. Looks more complicated (also, the regex starts to be complicated too...)

@jirikuncar
Copy link
Contributor

But you know which is the last line of the bullet point so you can just check that one.

@@ -142,16 +144,22 @@ def _check_bullets(lines, **kwargs):
max_length = kwargs.get("max_length", 72)
labels = {l for l, _ in kwargs.get("commit_msg_labels", tuple())}

def _strip_ticket_directives(line):
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm it looks fine.

@jirikuncar
Copy link
Contributor

Cool. I think we can integrate it. Thanks a lot for quick replies!

@@ -142,16 +144,22 @@ def _check_bullets(lines, **kwargs):
max_length = kwargs.get("max_length", 72)
labels = {l for l, _ in kwargs.get("commit_msg_labels", tuple())}

def _strip_ticket_directives(line):
return re.sub(r' \([^)]*\)$', '', line)
Copy link
Contributor

Choose a reason for hiding this comment

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

there might me several * Foo bar. (closes #1) (addresses #2)

Copy link
Contributor

Choose a reason for hiding this comment

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

* -> \{1,\}

* Adds a check for the trailing dots in the bullets of the commit
  message.

Signed-off-by: David Caro <david@dcaro.es>
@david-caro
Copy link
Contributor Author

Found also another issue, added tests for it too

@jirikuncar jirikuncar merged commit 2ab1f74 into inveniosoftware-attic:master Jul 18, 2016
@david-caro
Copy link
Contributor Author

Is there anything else I should address?

@david-caro
Copy link
Contributor Author

oops... did not refresh the page 🙈

@jirikuncar
Copy link
Contributor

@david-caro if you have something just open new PR.

@david-caro david-caro deleted the add_dot_check branch July 18, 2016 14:57
@david-caro david-caro restored the add_dot_check branch July 18, 2016 14:57
@david-caro david-caro deleted the add_dot_check branch July 18, 2016 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants