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

Add support for .vue files #145

Merged
merged 14 commits into from
Jun 8, 2021
Merged
21 changes: 21 additions & 0 deletions matchers/matchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/preslavmihaylov/todocheck/matchers/scripts"
"github.com/preslavmihaylov/todocheck/matchers/standard"
"github.com/preslavmihaylov/todocheck/matchers/state"
"github.com/preslavmihaylov/todocheck/matchers/vue"
)

// TodoMatcher for todo comments
Expand Down Expand Up @@ -124,6 +125,23 @@ var (
return groovy.NewCommentMatcher(callback)
},
}

vueMatcherFactory = &matcherFactory{
func() func([]string) TodoMatcher {
var once sync.Once
var matcher TodoMatcher

return func(customTodos []string) TodoMatcher {
once.Do(func() {
matcher = vue.NewTodoMatcher(customTodos)
})
return matcher
}
}(),
func(callback state.CommentCallback) CommentMatcher {
return vue.NewCommentMatcher(callback)
},
}
)

var supportedMatchers = map[string]*matcherFactory{
Expand Down Expand Up @@ -159,6 +177,9 @@ var supportedMatchers = map[string]*matcherFactory{

// file types, supporting python comments
".py": pythonMatcherFactory,

// file types, supporting js, html and css comments
".vue": vueMatcherFactory,
}

// TodoMatcherForFile gets the correct todo matcher for the given filename
Expand Down
138 changes: 138 additions & 0 deletions matchers/vue/comments.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
package vue

import (
"github.com/preslavmihaylov/todocheck/matchers/state"
)

// NewCommentMatcher for vue comments
func NewCommentMatcher(callback state.CommentCallback) *CommentMatcher {
return &CommentMatcher{
callback: callback,
}
}

// CommentMatcher for vue comments
type CommentMatcher struct {
callback state.CommentCallback
buffer string
lines []string
linecnt int
stringToken rune
isExitingMultilineComment bool
commentType string
isStartingHTML bool
}

// NonCommentState for vue comments
func (m *CommentMatcher) NonCommentState(
filename, line string, linecnt int, prevToken, currToken, nextToken rune,
) (state.CommentState, error) {
if prevToken == '/' && currToken == '/' {
m.buffer += string(currToken)

return state.SingleLineComment, nil
} else if currToken == '"' || currToken == '\'' {
preslavmihaylov marked this conversation as resolved.
Show resolved Hide resolved
m.stringToken = currToken

return state.String, nil
} else if prevToken == '/' && currToken == '*' {
m.buffer += string(currToken)
m.commentType = "CSS"

return state.MultiLineComment, nil
} else if prevToken == '<' && currToken == '!' && nextToken == '-' {
m.isStartingHTML = true

return state.NonComment, nil
} else if m.isStartingHTML && nextToken == '-' {
m.buffer += "<!-"
m.commentType = "HTML"

return state.MultiLineComment, nil
} else if m.isStartingHTML && nextToken != '-' {
m.isStartingHTML = false
}

return state.NonComment, nil
}

// StringState for vue comments
func (m *CommentMatcher) StringState(
filename, line string, linecnt int, prevToken, currToken, nextToken rune,
) (state.CommentState, error) {
if prevToken != '\\' && currToken == m.stringToken {
return state.NonComment, nil
}

return state.String, nil
}

// SingleLineCommentState for vue comments
func (m *CommentMatcher) SingleLineCommentState(
filename, line string, linecnt int, prevToken, currToken, nextToken rune,
) (state.CommentState, error) {
if currToken == '\n' {
err := m.callback(m.buffer, filename, []string{line}, linecnt)
if err != nil {
return state.NonComment, err
}

m.resetState()
return state.NonComment, nil
}

m.buffer += string(currToken)
return state.SingleLineComment, nil
}

// MultiLineCommentState for vue comments
func (m *CommentMatcher) MultiLineCommentState(
filename, line string, linecnt int, prevToken, currToken, nextToken rune,
) (state.CommentState, error) {
if m.isExitingMultilineComment {
m.resetState()
return state.NonComment, nil
}

m.buffer += string(currToken)
if isEndOfMultilineComment(m.commentType, prevToken, currToken, nextToken) {
m.buffer += string(nextToken)
err := m.callback(m.buffer, filename, m.lines, m.linecnt)
if err != nil {
return state.NonComment, err
}

m.isExitingMultilineComment = true
return state.MultiLineComment, nil
}

if prevToken == '\n' {
m.lines = append(m.lines, line)
}

return state.MultiLineComment, nil
}

func (m *CommentMatcher) resetState() {
m.buffer = ""
m.lines = nil
m.linecnt = 0
m.stringToken = 0
m.isExitingMultilineComment = false
m.commentType = ""
m.isStartingHTML = false
}

func isEndOfMultilineComment(commentType string, prev, curr, next rune) bool {
if commentType == "CSS" {
if curr == '*' && next == '/' {
return true
}
}
if commentType == "HTML" {
if prev == '-' && curr == '-' && next == '>' {
return true
}
}
return false
}
3 changes: 3 additions & 0 deletions matchers/vue/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// Package vue contains a todo matcher & comments matcher for vue comments.
// Vue single-line comments use the '//' literal, and the multi-line comments use the CS&JS /**/ or the HTML <!-- --> literals.
package vue
62 changes: 62 additions & 0 deletions matchers/vue/todomatcher.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package vue

import (
"regexp"

"github.com/preslavmihaylov/todocheck/common"
"github.com/preslavmihaylov/todocheck/matchers/errors"
)

// NewTodoMatcher for vue comments
func NewTodoMatcher(todos []string) *TodoMatcher {
pattern := common.ArrayAsRegexAnyMatchExpression(todos)

singleLineTodoPattern := regexp.MustCompile(`^\s*//.*` + pattern)
singleLineValidTodoPattern := regexp.MustCompile(`^\s*// ` + pattern + ` (#?[a-zA-Z0-9\-]+):.*`)

multiLineTodoPattern := regexp.MustCompile(`(?s)^\s*(<\!--|/*).*` + pattern)
multiLineValidTodoPattern := regexp.MustCompile(`(?s)^\s*(<\!--|/*).*` + pattern + ` (#?[a-zA-Z0-9\-]+):.*`)

return &TodoMatcher{
singleLineTodoPattern: singleLineTodoPattern,
singleLineValidTodoPattern: singleLineValidTodoPattern,
multiLineTodoPattern: multiLineTodoPattern,
multiLineValidTodoPattern: multiLineValidTodoPattern,
}
}

// TodoMatcher for vue comments
type TodoMatcher struct {
singleLineTodoPattern *regexp.Regexp
singleLineValidTodoPattern *regexp.Regexp
multiLineTodoPattern *regexp.Regexp
multiLineValidTodoPattern *regexp.Regexp
}

// IsMatch checks if the current expression matches a vue comment
func (m *TodoMatcher) IsMatch(expr string) bool {
return m.singleLineTodoPattern.Match([]byte(expr)) || m.multiLineTodoPattern.Match([]byte(expr))
}

// IsValid checks if the expression is a valid todo comment
func (m *TodoMatcher) IsValid(expr string) bool {
return m.singleLineValidTodoPattern.Match([]byte(expr)) || m.multiLineValidTodoPattern.Match([]byte(expr))
}

// ExtractIssueRef from the given expression.
// If the expression is invalid, an ErrInvalidTODO is returned
func (m *TodoMatcher) ExtractIssueRef(expr string) (string, error) {
if !m.IsValid(expr) {
return "", errors.ErrInvalidTODO
}

singleLineRes := m.singleLineValidTodoPattern.FindStringSubmatch(expr)
multiLineRes := m.multiLineValidTodoPattern.FindStringSubmatch(expr)
if len(singleLineRes) >= 2 {
return singleLineRes[1], nil
} else if len(multiLineRes) >= 3 {
return multiLineRes[2], nil
}

panic("Invariant violated. No issue reference found in valid TODO")
}
51 changes: 51 additions & 0 deletions testing/scenarios/vue/main.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// oneline comment, malformed TODO
// TODO 1: oneline comment wellformed
// TODO 2: oneline comment with Issue closed

<!-- TODO 3: wellformed HTML multiline entry -->
<!-- TODO 2: wellformed HTML multiline entry, BUT issue closed -->
<!-- TODO: missing number -->

/* TODO 1: wellformed CS/JS multiline entry */
/* TODO 2: wellformed CS/JS multiline entry, BUT issue closed */
/*
wellformed CS/JS multline entry
*/

/*
TODO: malformed CS/JS multline entry, missing number
*/

<!--
TODO 4: wellformed HTML multiline entry
-->

FelixTheodor marked this conversation as resolved.
Show resolved Hide resolved

"this is a // TODO 2: valid comment in a string with issue closed"
"this is a /* TODO 2: valid multiline comment in a string with issue closed */"
'this is a <!-- TODO 2: valid HTML multiline comment in a string with issue closed -->'

<template>
<div id="app">
{{ message }}
</div>
</template> /* TODO 1: valid multiline comment in code */

<script>
export default {
data() {
return {
message: 'Hello World',
};
},
};
</script> <!-- TODO 1:
valid HTML multiline comment in code -->

<style>
#app {
font-size: 18px;
font-family: 'Roboto', sans-serif;
color: blue; // TODO 2: online comment wellformed in code, BUT issue closed
}
</style>
49 changes: 49 additions & 0 deletions testing/todocheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,55 @@ func TestPrintingVersionFlagStopsProgram(t *testing.T) {
}
}

func TestVueTodos(t *testing.T) {
err := scenariobuilder.NewScenario().
WithBinary("../todocheck").
WithBasepath("./scenarios/vue").
WithConfig("./test_configs/no_issue_tracker.yaml").
WithIssueTracker(issuetracker.Jira).
WithIssue("1", issuetracker.StatusOpen).
WithIssue("2", issuetracker.StatusClosed).
WithIssue("3", issuetracker.StatusOpen).
WithIssue("4", issuetracker.StatusOpen).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeMalformed).
WithLocation("scenarios/vue/main.vue", 1).
ExpectLine("// oneline comment, malformed TODO")).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeIssueClosed).
WithLocation("scenarios/vue/main.vue", 3).
ExpectLine("// TODO 2: oneline comment with Issue closed")).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeIssueClosed).
WithLocation("scenarios/vue/main.vue", 6)).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeMalformed).
WithLocation("scenarios/vue/main.vue", 7)).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeIssueClosed).
WithLocation("scenarios/vue/main.vue", 10)).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeMalformed).
WithLocation("scenarios/vue/main.vue", 0).
ExpectLine("TODO: malformed CS/JS multline entry, missing number").
Copy link
Owner

Choose a reason for hiding this comment

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

Why is the location of this line 0?

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be:

WithLocation("scenarios/vue/main.vue", 15).
ExpectLine("/*").
ExpectLine("TODO: ...").
ExpectLine("*/").

Copy link
Contributor Author

@FelixTheodor FelixTheodor Jun 1, 2021

Choose a reason for hiding this comment

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

Oh, this is weird. I mean, you are right, but I get an error if I implement it like you proposed:

` No matching todo detected in any of the scenarios

    Actual:
    ERROR: Malformed todo
    scenarios/vue/main.vue:0: TODO: malformed CS/JS multline entry, missing number
    scenarios/vue/main.vue:1: */
    	> TODO should match pattern - TODO {task_id}:
    
    Remaining scenarios:
    (scenario #1)
    ERROR: Malformed todo
    scenarios/vue/main.vue:15: /*
    scenarios/vue/main.vue:16: TODO: ...
    scenarios/vue/main.vue:17: */
    	> TODO should match pattern - TODO {task_id}:
    
    (scenario #2)
    ERROR: Issue is closed
    scenarios/vue/main.vue:49:   color: blue; // TODO 2: online comment wellformed in code, BUT issue closed
    `

And here, the displayed line of the actual todo is zero. I'm confused. Do you have any idea why it behaves that way?

Copy link
Owner

Choose a reason for hiding this comment

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

This actually is a bug with the comment matcher implementation.

There are two issues:

  • You are not matching the multiline css comments correctly. You should match /* some todo comment */, but instead you are matching some todo comment */
  • You are not tracking the linecnt at which a multiline comment begins. It should be persisted when transitioning to the multiline comment state similar to how it's done here and later used here

Copy link
Owner

Choose a reason for hiding this comment

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

You can try reproducing this by:

  • go build todocheck and copying the output binary to a comfortable location
  • Adding a new file main.vue which contains the scenario from the test
  • Add a new .todocheck.yaml file with contents:
origin: https://github.com/todocheck-tests/test-public
issue_tracker: GITHUB
  • run ./todocheck & verify the results

You can try instrumenting some debug prints to see what's going on or use a standard debugger if you have one set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, you were right, it was indeed a bug. I think the new commits I pushed fix it. At least, now all the testing works as expected. I also added another test for an invalid HTML multiline TODO, which also works.

But there is one other thing I noticed, and I am not sure if its a feature or a bug: The last line of every file is always ignored, also by other file matchers (checked PHP briefly). This means that multiline comments that are closed in the last line are not checked for errors (since they technically never become closed).

If this is indeed a feature, I'd be interested in an explanation for it. Maybe it would then be helpful to throw an error if the last line is not empty, since it could lead to missing TODOs otherwise?

Copy link
Owner

Choose a reason for hiding this comment

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

But there is one other thing I noticed, and I am not sure if its a feature or a bug: The last line of every file is always
ignored, also by other file matchers (checked PHP briefly). This means that multiline comments that are closed in the
last line are not checked for errors (since they technically never become closed).

The final testcase you've added:

		ExpectTodoErr(
			scenariobuilder.NewTodoErr().
				WithType(errors.TODOErrTypeMalformed).
				WithLocation("scenarios/vue/main.vue", 53).
				ExpectLine("<!--").
				ExpectLine("TODO : malformed HTML multiline entry").
				ExpectLine("-->")).

proves it otherwise. What exactly do you mean?

If your observation is indeed correct, then this is a bug. If you can reproduce it deterministically, could you open a bug with reproduction steps & share the details?

ExpectLine("*/")).
ExpectTodoErr(
scenariobuilder.NewTodoErr().
WithType(errors.TODOErrTypeIssueClosed).
WithLocation("scenarios/vue/main.vue", 49).
ExpectLine(" color: blue; // TODO 2: online comment wellformed in code, BUT issue closed")).
Run()
if err != nil {
t.Errorf("%s", err)
}
}

// Testing multiple todo matchers created for different file types
func TestMultipleTodoMatchers(t *testing.T) {
err := scenariobuilder.NewScenario().
Expand Down