<p><b>@cbeams</b> requested changes on this pull request.</p>
<p>This was a very quick review from my side. Mostly style nits, but my question about the removal of gradle-witness data should be addressed before approval.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244397705">gradle/witness/gradle-witness.gradle</a>:</p>
<pre style='color:#555'>> - 'javax.inject:javax.inject:91c77044a50c481636c32d916fd89c9118a72195390452c81065080f957de7ff',
- 'aopalliance:aopalliance:0addec670fedcd3f113c5c8091d783280d23f75e3acb841b61a9cdb079376a08',
- 'com.lambdaworks:scrypt:9a82d218099fb14c10c0e86e7eefeebd8c104de920acdc47b8b4b7a686fb73b4',
- 'com.google.zxing:core:11aae8fd974ab25faa8208be50468eb12349cd239e93e7c797377fa13e381729',
- 'com.cedricwalter:tor-binary-geoip:fbd7656a262607e5a73016e048d5270cbabcd4639a1795b4b4e762df8877429d',
- 'com.github.JesusMcCloud:jtorctl:ba71601cbe50474ccc39a17bc6f7880c1412d8d19b94d37aee69ea2917f72046',
- 'org.apache.commons:commons-compress:5f2df1e467825e4cac5996d44890c4201c000b43c0b23cffc0782d28a0beb9b0',
- 'org.tukaani:xz:a594643d73cc01928cf6ca5ce100e094ea9d73af760a5d4fb6b75fa673ecec96',
- 'com.madgag.spongycastle:core:8d6240b974b0aca4d3da9c7dd44d42339d8a374358aca5fc98e50a995764511f',
- 'net.jcip:jcip-annotations:be5805392060c71474bf6c9a67a099471274d30b83eef84bfc4e0889a4f1dcc0',
- 'org.bitcoinj:orchid:f836325cfa0466a011cb755c9b0fee6368487a2352eb45f4306ad9e4c18de080',
- 'com.squareup.okhttp:okhttp:b4c943138fcef2bcc9d2006b2250c4aabbedeafc5947ed7c0af7fd103ceb2707',
- 'org.jetbrains.kotlin:kotlin-stdlib-common:4b161ef619eee0d1a49b1c4f0c4a8e46f4e342573efd8e0106a765f47475fe39',
- 'com.squareup.okio:okio:114bdc1f47338a68bcbc95abf2f5cdc72beeec91812f2fcd7b521c1937876266',
- ]
-}
</pre>
<p>Was this removal intentional?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244397877">monitor/src/main/java/bisq/monitor/Configurable.java</a>:</p>
<pre style='color:#555'>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+import java.util.Properties;
+
+/**
+ * Configurable is configurable.
+ *
+ * @author Florian Reimair
+ */
</pre>
<p>I'd drop any boilerplate Javadoc like this and those that follow. Fine to have it if it really explains something, but otherwise it's line noise.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398103">monitor/src/main/java/bisq/monitor/Configurable.java</a>:</p>
<pre style='color:#555'>> + * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+import java.util.Properties;
+
+/**
+ * Configurable is configurable.
+ *
+ * @author Florian Reimair
+ */
+public abstract class Configurable {
+
+
</pre>
<p>No need for double blank lines here</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398224">monitor/src/test/java/bisq/monitor/MonitorInfrastructureTests.java</a>:</p>
<pre style='color:#555'>> + * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+
</pre>
<p>No need for double blank lines here</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398265">monitor/src/test/java/bisq/monitor/TorHiddenServiceStartupTimeTests.java</a>:</p>
<pre style='color:#555'>> +import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import bisq.monitor.metric.TorHiddenServiceStartupTime;
+
+public class TorHiddenServiceStartupTimeTests {
+
+ private class DummyReporter extends Reporter {
+
+ private long result;
+
+ @Override
+ public void report(long value) {
+ result = value;
+
</pre>
⬇️ Suggested change
<pre style="color: #555">-
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398416">monitor/src/test/java/bisq/monitor/TorHiddenServiceStartupTimeTests.java</a>:</p>
<pre style='color:#555'>> + report(Long.parseLong(values.values().iterator().next()));
+ }
+
+ @Override
+ public void report(Map<String, String> values, String prefix) {
+ report(values);
+ }
+
+ @Override
+ public void report(long value, String prefix) {
+ report(value);
+ }
+
+ }
+
+ private final static File torWorkingDirectory = new File(TorHiddenServiceStartupTimeTests.class.getSimpleName());
</pre>
<p>Please declare fields together at top of class, not interspersed throughout methods.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398442">monitor/src/test/java/bisq/monitor/TorStartupTimeTests.java</a>:</p>
<pre style='color:#555'>> + * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+
</pre>
<p>No need for double blank lines here</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2181#discussion_r244398784">build.gradle</a>:</p>
<pre style='color:#555'>>
dependencies {
- compile project(':core')
- compile "com.sparkjava:spark-core:$sparkVersion"
- compile 'net.gpedro.integrations.slack:slack-webhook:1.1.1'
+ compile 'org.slf4j:slf4j-api:1.7.22'
+ compile 'ch.qos.logback:logback-core:1.1.10'
+ compile 'ch.qos.logback:logback-classic:1.1.10'
</pre>
<p>Since these dependencies are now repeated between <code>:monitor</code> and <code>:common</code>, their version numbers can be extracted to <code>$sf4jVersion</code> and <code>$logbackVersion</code> to avoid version drift between the two subprojects.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/2181#pullrequestreview-188361166">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtop0pfP3QSDeySV6cjee1o4tSS6Vks5u9ns6gaJpZM4ZkH0M">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtiPQfMfITOhYGLPsunFxH6QLJjrPks5u9ns6gaJpZM4ZkH0M.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@cbeams requested changes on #2181"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2181#pullrequestreview-188361166"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2181#pullrequestreview-188361166",
"url": "https://github.com/bisq-network/bisq/pull/2181#pullrequestreview-188361166",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>