-
Notifications
You must be signed in to change notification settings - Fork 0
Preprocessor (•)(•) #21
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
Conversation
bialger
left a comment
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.
Generally a good job. However, there are some code style issues and a problem with ISP at import processor, I recommend separating the graph thing from the rest of the class. And also I would do the thing with include paths in the UI method. All in all, there are some good decisions, but some changes would not hurt.
bialger
left a comment
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.
All in all a good job! However, I think there is room for improvement: mostly code style, but I suggest using more expandable design in the directive processor. We can merge it after all of these changes are done.
…cy in preprocessor tests
… for submodule management
Add preprocessor tests
bialger
left a comment
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.
Everything looks fine, a good job!
…ecks and ignore cache files
…apper for multi-operator and keyword checks
sashbek
left a comment
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.
Вроде с кайфом
No description provided.