RFR: 2331: Improve review notes

Pavel Rappo prappo at openjdk.org
Thu Jul 18 20:23:47 UTC 2024


On Thu, 18 Jul 2024 19:58:58 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> I was just thinking we should make the minimal necessary changes.  I admit that parsing text displayed to the end user isn't best practice, but we do it in many places of SKARA.
>
> It's just a suggestion, since you don't like it and I am ok with your pr, I will approve it now.

Usually, making a minimally necessary change is good, and parsing end-user text is bad. In fact, I'd go to great lengths to avoid "stringly typed" code that operates on strings that are also subject to change.

Here, I initially thought I might be violating some encapsulation boundary by bringing a dependency. Maybe I still am. If nothing else, it's not the first class imported from that package:

     import org.openjdk.skara.jcheck.JCheckConfiguration;
    +import org.openjdk.skara.jcheck.TooFewReviewersIssue;

-------------

PR Review Comment: https://git.openjdk.org/skara/pull/1679#discussion_r1683433452


More information about the skara-dev mailing list