-
Notifications
You must be signed in to change notification settings - Fork 130
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
Default tester config should not be accepted as a good configuration #200
Comments
I think this would be enough. If contributors continue facing problems running default config we will add new logic to examine contents. In addition, we should update README.md to point that default config must be changed. |
It is not good to break/damage a config. Situation that you see with students is problem of person that sit in front of PC and even do not ask himself a question "How tool can get to know what I am testing ?" . The only point to change is to make red note in README to change a config up to user tested scenario. |
Yes. Maybe we should reconsider having the configuration on the specific project and think of moving it to the main index, as all projects use the same configs. To do this, would need to take the index generation out of groovy probably and have |
I think that checkstyle own sources should be a mandatory project to generate reports for.
Hm, maybe we can pass two dirs location to patch-diff-report-tool: one which contains reports before changes and one which contains reports after. So, patch-diff-report-tool should take care of summary index.html generation. |
That is good that all agree on README update. I do not like idea of extra customization of index-summary page. We will never force person to do all correctly if he is completely inexperienced. They do not even read reports vilations before publication. Nothing will help them. What we can do to ease detection of bad reports us to make config to produce completely no violations. Default config already has a simple and not frequent problem detection. We could update it to never find a problem - weird config. So user will generate report with no violations at all - first sign that smth is wrong. |
…oduce no violations
…oduce no violations
…oduce no violations
…oduce no violation
…oduce no violation
…oduce no violation
Fix is merged |
Taken from checkstyle/checkstyle#4060 (comment) and checkstyle/checkstyle#3967 (comment) and checkstyle/checkstyle#3928 (comment) and checkstyle/checkstyle#3887 (comment) and so on ....,
https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/my_check.xml#L26
Users run regression and report no differences but it turns out their reports are false because they didn't change the configuration file for their checks.
Tester shouldn't accept this configuration file. Either we should change it to have an invalid module name, or no default module and examine contents somehow to determine if user specified module or not.
If module isn't added/changed, we should result in an error so it is clear to novice users.
The text was updated successfully, but these errors were encountered: