Github user amihalik commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148799979
--- Diff:
extras/periodic.notification/api/src/main/java/org/apache/rya/periodic/notification/api/PeriodicNotificationClient.java
---
@@ -36,29 +36,26 @@
* Adds a new notification to be registered with the {@link
NotificationCoordinatorExecutor}
* @param notification - notification to be added
*/
- public void addNotification(PeriodicNotification notification);
--- End diff --
tldr: I think we should pull the PR as is, but don't perform "style
changes" and "feature changes" in a single PR in the future.
I didn't know that the modifier was redundant. I'd keep in the "public"
modifiers to be consistent with the code base, but I'm fine with the change
since it is consistent within the sub module.
Overall, I think there are two key things to keep in mind:
1. Style changes may turn off reviewers from looking at your "feature
change" PR. Style changes typically make your code commit a lot larger than
necessary and muddy important "feature changes" with a lot of style changes.
2. Defining a community style and tools to evaluate and enforce this style
is very important. Consistent style across the code base will lower the
barrier of entry for new developers, will build a larger community, etc. I
would enjoy reviewing and pulling those changes in. But I'd be equally turned
off if those style changes included "feature changes".
---
