Skip to content

Add @todo Fixing Functionality#123

Closed
adamzimmermann wants to merge 14 commits into
pfrenssen:8.x-3.xfrom
adamzimmermann:todo-fixer
Closed

Add @todo Fixing Functionality#123
adamzimmermann wants to merge 14 commits into
pfrenssen:8.x-3.xfrom
adamzimmermann:todo-fixer

Conversation

@adamzimmermann
Copy link
Copy Markdown
Contributor

@adamzimmermann
Copy link
Copy Markdown
Contributor Author

All tests and code style checks are passing.

@adamzimmermann
Copy link
Copy Markdown
Contributor Author

I'm not sure why this is showing old commits. If you are squashing when you merge, it shouldn't matter though I suppose. If you want me to open a new PR that only shows the new commits I can do that too. The diff looks correct though.

@adamzimmermann adamzimmermann marked this pull request as ready for review October 20, 2020 20:48
Copy link
Copy Markdown
Collaborator

@arkener arkener left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! The PHPStan check currently seems to be failing, I'm unsure why the check isn't showing up here, @klausi any idea?

Running this on core returns the expected results with 1 exception. The fixer currently breaks when an empty @todo is given, for example:

function lorem() {
  // @todo
  $node = Node::load(1);
}

Results in the following

function lorem() {
  // @todo $node = Node::load(1);
}

Could you add a test for the fixer? That way we can automatically validate the fixer. This can be achieved by adding the TodoCommentUnitTest.inc.fixed file, containing the expected result of the autofix based on the TodoCommentUnitTest.inc file.

@klausi
Copy link
Copy Markdown
Collaborator

klausi commented Oct 21, 2020

Strange that Travis CI does not show up. I also see some other commits here - can you make a new pull request where you start the branch fresh from origin/8.x-3.x?

@adamzimmermann
Copy link
Copy Markdown
Contributor Author

New PR with those fixes coming up.

@adamzimmermann
Copy link
Copy Markdown
Contributor Author

I fixed the new line creation, but how do we want to handle // @todo? Should that remain valid? i.e. we don't want to flag it at all? If we do flag it, we can't fix it unless we add some example text.

Maybe it should be fixed to @todo Issue description here.?

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.

4 participants