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

Incomplete attempt at @todo capitalization enforcement. #174

Draft
wants to merge 1 commit into
base: 8.3.x
Choose a base branch
from

Conversation

adamzimmermann
Copy link
Contributor

@adamzimmermann adamzimmermann commented Sep 19, 2022

@jonathan1055
Copy link
Contributor

On the d.o. issue you said

My vote is to take the gains we have and move forward to begin seeing the benefits.

Yes I agree. If I can help with anything, let me know

@klausi
Copy link
Collaborator

klausi commented Sep 20, 2022

Thanks, can you make an issue in the Coder issue queue? Then we can track credit and work there aside from the Drupal core issue.

@@ -7,9 +7,9 @@
* These are valid examples.
*
* @todo Valid.
* @todo valid with lower-case first letter
* @todo $can start with a $
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be valid, one could document a variable name like "@todo $entity could be NULL here, fix this later in foo_fcuntion()."

@@ -7,9 +7,9 @@
* These are valid examples.
*
* @todo Valid.
* @todo valid with lower-case first letter
* @todo $can start with a $
* @todo \also with backslash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, backslash should be valid. "@todo \Drupal should not be used here, implement dependency injection later".

@@ -142,7 +142,7 @@ private function checkTodoFormat(File $phpcsFile, int $stackPtr, string $comment
(?-i) # Reset to case-sensitive
(?! # Start another non-consuming look-ahead, this time negative
@todo\s # It has to match lower-case @todo followed by one space
(?!-|:)\S # and then any non-space except "-" or ":".
(?!-|:)[A-Z] # and then any capital letter except "-" or ":".
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is tricky. We want to warn about lower case characters, but allow everything else. Even lower case characters should be allowed in some cases, for example @todo some_function() is not a good name, refactor it to a better name.

I think we have code somewhere else in Coder that checks for such things in some comment standard, we should check that.

@jonathan1055
Copy link
Contributor

Yes, in response to @klausi's points above, I think the logic could be reverted to allow any non-space except "-" or ":" as we had before. Then we give a separate warning if the first letter is a lower-case alpha and provide the fix to convert to upper case.

@adamzimmermann
Copy link
Contributor Author

I have created an issue to facilitate further discussion as requested.

I tried to summarize the consensus there, but if others could chime in with their understanding of the requirements that would be great. Then I can begin working on implementing it.

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.

3 participants