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
[DRAFT] add protection from recursion #40
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: denis-tingaikin <denis.tingajkin@xored.com>
@@ -48,34 +49,29 @@ func (t *Target) ModTime() (time.Time, error) { | |||
} | |||
|
|||
type Analyzer struct { | |||
values map[string]Value | |||
values context.Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, using ctx in such way is anti-pattern
the code became more dirty
why do not just use the custom type over map?
var issue = a.Analyze(header("A {{ SOME-VALUE }} B")) | ||
require.NotNil(t, issue) | ||
require.Contains(t, issue.Message(), "recursion detected") | ||
issue = a.Analyze(header("A {{ SOME-VALUE }} C")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cosmetic: something I tried to fix in the last MR - the lack of whitespaces makes the tests difficult to understand
require.Contains(t, issue.Message(), "recursion detected") | ||
issue = a.Analyze(header("A {{ SOME-VALUE }} C")) | ||
require.NotNil(t, issue) | ||
require.Contains(t, issue.Message(), "recursion detected") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho, this is strange and resource ineffective to detect recursion on analyze stage:
- strange, because this error belongs to options/constructor, not method
- ineffective, because users will see the error for every analyzed package vs single misconfig error
Motivaion
Closes #32