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

List Mask (MASK) #1838

Closed
wants to merge 1 commit into from
Closed

List Mask (MASK) #1838

wants to merge 1 commit into from

Conversation

diablax
Copy link
Contributor

@diablax diablax commented Oct 29, 2018

Copy link
Member

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

NACK. Squash to a single commit per instructions at https://bisq.network/list-asset, and use blank lines per convention in import statements for the Mask class.

@diablax
Copy link
Contributor Author

diablax commented Nov 5, 2018

squashed and imports fixed.

@cbeams
Copy link
Member

cbeams commented Nov 5, 2018

@diablax
Copy link
Contributor Author

diablax commented Nov 5, 2018

My apologies Chris, I have now added the changes.

Copy link
Member

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

NACK, per suggested formatting changes.

assets/src/main/java/bisq/asset/coins/Mask.java Outdated Show resolved Hide resolved
assets/src/test/java/bisq/asset/coins/MaskTest.java Outdated Show resolved Hide resolved
assets/src/test/java/bisq/asset/coins/MaskTest.java Outdated Show resolved Hide resolved
assets/src/test/java/bisq/asset/coins/MaskTest.java Outdated Show resolved Hide resolved
@diablax
Copy link
Contributor Author

diablax commented Nov 5, 2018

Again, apologies. Did not notice these messy lines on nano, perhaps they were formatted differently on my machine.

@diablax
Copy link
Contributor Author

diablax commented Nov 6, 2018

Let me know if there is anything else to do.

@cbeams
Copy link
Member

cbeams commented Nov 6, 2018

From https://docs.bisq.network/exchange/howto/list-asset.html#step-6-submit-your-pull-request

image

The reason I need this is to clean up the body of your commit. You squashed it, but left all the comments from the squashed commits in the body of the remaining commit. The body should be empty. You can do that yourself and force push, or give me rights to do it. I'll merge afterward.

@diablax
Copy link
Contributor Author

diablax commented Nov 7, 2018

Feel free to amend. Edits are enabled.

@diablax
Copy link
Contributor Author

diablax commented Nov 7, 2018

Would it be easier to just submit a new PR?

@diablax
Copy link
Contributor Author

diablax commented Nov 7, 2018

I have cleaned up the code and will resubmit PR to comply with the rules.

@cbeams
Copy link
Member

cbeams commented Nov 7, 2018

Please just reopen this PR with a force-pushed cleaned up version of your commit. There's no need to open a new PR.

@cbeams
Copy link
Member

cbeams commented Nov 7, 2018

Wait. This PR now shows 2,085 files changed. Go ahead and resubmit unless you know how to fix that. Note that you submitted this PR from your master branch, which is something we ask you not to do in the instructions as well. This may be the source of the problem, especially if you pulled recent changes to upstream master.

@cbeams
Copy link
Member

cbeams commented Nov 7, 2018

It's because you deleted your fork of the repository.

@diablax
Copy link
Contributor Author

diablax commented Nov 7, 2018

Yes I will resubmit everything properly and make sure it is on a different branch. Apologies for taking up so much of your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants