Skip to content
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

Improve japanese bank branch validation #5694

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

ripcurlx
Copy link
Contributor

@ripcurlx ripcurlx commented Sep 13, 2021

Fixes #5688

@ripcurlx ripcurlx changed the title Fixes #5688. Improve japanese Bank branch validation Sep 13, 2021
@ripcurlx ripcurlx changed the title Improve japanese Bank branch validation Improve japanese bank branch validation Sep 13, 2021
@ripcurlx ripcurlx added this to the v1.7.4 milestone Sep 14, 2021
@ripcurlx ripcurlx requested a review from sqrrm September 14, 2021 07:16
@wiz
Copy link
Contributor

wiz commented Sep 14, 2021

The diff is so big I have no idea what you actually changed...

@wiz
Copy link
Contributor

wiz commented Sep 14, 2021

I think you should probably only change the bottom part of the file where the regex patterns actually changed, otherwise you're rewriting the entire code

@ripcurlx
Copy link
Contributor Author

ripcurlx commented Sep 14, 2021

I think you should probably only change the bottom part of the file where the regex patterns actually changed, otherwise you're rewriting the entire code

Did you look at the commits 😉. I've put the formatting fixes in a separate commit and the other two changes in two further commits. So it is easy to see what actually changed.

@Liisachan
Copy link

What's significant is the 2nd commit c781394. It's so simple, no regression seems possible. The 3rd commit 4d62136 is even more trivial, like just improving comments.

The 1st commit 851ef07 is less transparent, but after hiding whitespace changes, I see there are only 2 actual edits:

  1. import java.util.HashMap; and import bisq.core.locale.Country; deleted.
  2. new ArrayList<String>(); -> new ArrayList<>();

I guess 1 is simply unused "include"s, and 2 is something like auto in C++? Idk, a real Java programmer can review this.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting and style changes are good. I cannot tell if the regex changes are good but from @wiz and @Liisachan they sound correct.

@sqrrm
Copy link
Member

sqrrm commented Sep 14, 2021

utACK

@sqrrm sqrrm merged commit 0e2da28 into bisq-network:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

japanese.validation.regex should accept U+3007
4 participants