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

BUG: Fix incorrect error message in vtkMRMLColorTableNode #6729

Merged
merged 1 commit into from Dec 12, 2022

Conversation

pieper
Copy link
Member

@pieper pieper commented Dec 11, 2022

Typo in error message makes debugging more difficult

@pieper pieper requested a review from lassoan December 11, 2022 18:09
@pieper
Copy link
Member Author

pieper commented Dec 11, 2022

I make this one character fix using the github web editor and it seems that the commit message shows up in a way that fails the check. I think it's actually fine to ignore unless someone knows how to fix it.

@jamesobutler
Copy link
Contributor

@pieper re https://slicer.readthedocs.io/en/latest/developer_guide/style_guide.html#commits, the subject line is not capitalized because after the commit prefix it is fix and not Fix.

Here it describes the commit prefix, followed by a colon, followed by a space, followed by a capitalized A-Z character.

pattern: '^(ENH|PERF|BUG|STYLE|DOC|COMP): ([A-Z])+'

BUG: fix incorrect error message fails the regex at
BUG: f

@jamesobutler
Copy link
Contributor

jamesobutler commented Dec 11, 2022

This confusion was previously reported in #6692, so we could try and improve the example message to better indicate what the regex means. If enforcing the capitalization is not desired, then updating the Slicer docs about contributing to remove the note that the subject line should be capitalized.

@pieper
Copy link
Member Author

pieper commented Dec 11, 2022

Wow, thanks for pointing that out @jamesobutler. I did not look at the regexp carefully. I read the error message and also the link to the Slicer documentation and never thought capitalization would be the issue. The current message is misleading IMO, since it explicitly complains about the "type" being wrong. I'll make a PR to change the message (and hope I get the commit message formatted correctly!).

image

I'm not sure starting with a capital letter should be a hard requirement but I'll ignore that for now. If someone will provide the review approval the commit message can be fixed during the squash and merge I believe.

pieper added a commit that referenced this pull request Dec 11, 2022
jcfr pushed a commit that referenced this pull request Dec 12, 2022
See:
#6729
and
#6692

Co-authored-by: James Butler <jamesobutler@users.noreply.github.com>
jcfr pushed a commit that referenced this pull request Dec 12, 2022
See:
#6729
and
#6692

Co-authored-by: James Butler <jamesobutler@users.noreply.github.com>
Typo in error message makes debugging more difficult
@jcfr jcfr merged commit 3d62cbf into main Dec 12, 2022
@jcfr jcfr deleted the fix-colornode-message branch December 12, 2022 01:20
@jcfr jcfr changed the title BUG: fix incorrect error message BUG: Fix incorrect error message in vtkMRMLColorTableNode Jan 23, 2023
@jcfr jcfr added this to the Slicer 5.2.2 milestone Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants