Testing: Improve instructions and process #8844 #8977
Conversation
…ove-build-process # Conflicts: # src/test/java/teammates/test/pageobjects/Browser.java
…ove-build-process # Conflicts: # src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java
There was a problem hiding this comment.
I don't know which PR to review since you closed all of them so I suppose I will review this.
Let's add something like IllegalImport in CheckStyle to disallow DecimalFormat and DateTimeFormatter with custom messages.
See if it's possible to disallow fully qualified classes too (I don't think so).
| /** | ||
| * Returns DateTimeFormatter instance with Locale.US as default locale. | ||
| */ | ||
| public static DateTimeFormatter ofPattern(String pattern) { |
There was a problem hiding this comment.
Return a TeammatesDateTimeFormatter instead.
Also, add a format method that we can use instead.
There was a problem hiding this comment.
Return a TeammatesDateTimeFormatter instead.
How exactly?
My idea is that since we set the locale while setting the pattern, I will only wrap that function so that after that function returns the DateTimeFormatter we can use other DateTimeFormatter functions with the required set locale.
This will save us from the hassle of wrapping every function and will make sure code works once TeammatesDateTimeFormatter.ofPattern has been used
Similarly for this https://github.com/TEAMMATES/teammates/pull/8977/files#r204227822
There was a problem hiding this comment.
@LiHaoTan Need your input on this
There was a problem hiding this comment.
My idea is that it should wrap every function (like a forwarding class) precisely to prevent unexpected behaviors.
For example one could do TeammatesDateTimeFormatter.ofPattern(...).withLocale(...); and that is not something we want.
Anyway in any case we don't have to wrap every function if we don't treat TeammatesDateTimeFormatter as an extension of DateTimeFormatter but rather the class is just a formatter and the fact that it uses DateTimeFormatter is merely an implementation detail.
| import java.util.Locale; | ||
|
|
||
| /** | ||
| * DateTimeFormatter for TEAMMATES with default Locale.US settings. |
There was a problem hiding this comment.
Something like this may be better?
DateTimeFormatter with TEAMMATES specific defaults (i.e. US locale).
| @@ -211,6 +213,7 @@ private String getInstructorQuestionResultsStatisticsHtml( | |||
| df.setMinimumFractionDigits(0); | |||
| df.setMaximumFractionDigits(5); | |||
| df.setRoundingMode(RoundingMode.DOWN); | |||
There was a problem hiding this comment.
Looks like StringHelper.toDecimalFormatString is not flexible enough. Let's create a TeammatesDecimalFormat for that?
Fixes #8844
Ongoing.
@LiHaoTan Need your input on the Intellij
use default reporterWould appreciate if someone can try out the following for me and check if it works on EclipseOutline of Solution
trueso that its standardized for IntelliJNOTE: Continued from #8845 as I messed up the old PR :P