-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
WIP: ForwardingService refactoring for JDK 11 with record-like Config
class.
#3378
Open
msgilligan
wants to merge
10
commits into
bitcoinj:master
Choose a base branch
from
msgilligan:msgilligan/ForwardingService-refactor-record-like
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
WIP: ForwardingService refactoring for JDK 11 with record-like Config
class.
#3378
msgilligan
wants to merge
10
commits into
bitcoinj:master
from
msgilligan:msgilligan/ForwardingService-refactor-record-like
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msgilligan
commented
Apr 26, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't review my own PR, but I found some things that need changing.
integration-test/build.gradle
Outdated
compileJava.options.encoding = 'UTF-8' | ||
compileTestJava.options.encoding = 'UTF-8' | ||
javadoc.options.encoding = 'UTF-8' | ||
|
||
compileJava { | ||
options.compilerArgs << '-Xlint:deprecation' | ||
options.compilerArgs.addAll(['--release', '17']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
integration-test/src/test/java/org/bitcoinj/examples/ForwardingServiceTest.java
Outdated
Show resolved
Hide resolved
msgilligan
force-pushed
the
msgilligan/ForwardingService-refactor-record-like
branch
2 times, most recently
from
April 27, 2024 20:24
ff4efcc
to
547c8c8
Compare
* examples, integration-test: Gradle use JDK 17+ * ForwardingService: Use Java 17 to simplify Simplification: * Move more code back to main method * Use record class to collect config params * static parse(args) method to build config * Construct ForwardingService with Wallet and Config objects * Use effectively final objects in the main try-with-resources * Every variable -- except two locals in parse() -- is either final or effectively final. * Keep important stuff towards the top of the file, details at the bottom. The overall objective of this refactoring was to make the overall flow clearly visible in the main method of the app while preserving ForwardingService as a "reusable" and `Closable` object. I realize that things could be further simplified by moving more code into main, but I want this example to encourage creating modular "services" with bitcoinj.
This also allows moving two constants inside the record class.
Also add BitcoinNetwork.supports(Address)
Rewrite forward() to be a more generic send() method (which is essentially just a wrapper to wallet.sendTransaction() that logs various completion states. Note that we moved the whenComplete blog that logs "relayed" status so users can use this code in more cases. This is because a common question that comes up is "why was my transaction not seen?". So making this logging code more reusable should be helpful.
msgilligan
force-pushed
the
msgilligan/ForwardingService-refactor-record-like
branch
from
May 14, 2024 18:32
547c8c8
to
48e09ac
Compare
ItoroD
reviewed
May 18, 2024
examples/src/main/java/org/bitcoinj/examples/ForwardingService.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is WIP for an improved JDK 11
ForwardingService
Overall this results in (IMHO) a very clean example:
main()
method is short but clearly shows overall flow of the examplemain()
method includes all (main thread) stdout. (The other stdout is really log functionality, but since this example doesn't use a logging framework were using stdout)record
class