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

refactor: update snippets to be compatible with xrpl.js #1706

Merged
merged 23 commits into from
Oct 19, 2021

Conversation

khancode
Copy link
Collaborator

@khancode khancode commented Oct 7, 2021

High Level Overview of Change

Update snippets to be compatible with xrpl.js

Context of Change

Updated getTransaction & paths snippets to be compatible with xrpl.js.

Type of Change

  • Refactor (non-breaking change that only restructures code)

Test Plan

Manually tested snippets to ensure they are functional.

@khancode khancode changed the title Update snippets Update snippets to be compatible with xrpl.js Oct 7, 2021
@khancode khancode changed the title Update snippets to be compatible with xrpl.js refactor: update snippets to be compatible with xrpl.js Oct 8, 2021
@natenichols natenichols added the documentation Issues/pull requests that are primarily related to documentation label Oct 15, 2021
@khancode khancode changed the base branch from develop to mj/snippets October 18, 2021 21:21
@khancode khancode requested a review from mukulljangid October 18, 2021 21:21
Copy link
Collaborator

@mvadari mvadari left a comment

Choose a reason for hiding this comment

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

looks like there's some git issues with a different branch, so I'll hold off on approval until that's resolved

@natenichols
Copy link
Contributor

Looks like you have some old merge commits.

you should git rebase -i --rebase-merges develop and drop the old merge commits.

Copy link
Contributor

@natenichols natenichols left a comment

Choose a reason for hiding this comment

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

Snippets look great.

Once the merge issues are resolved, feel free to merge the snippets.

You could also

git checkout develop
 # switch to tmp branch
git checkout -b tmp
# Bring your changes in ./snippets to temp branch
git checkout update-snippets -- ./snippets
# back to this branch
git checkout update-snippets
# make this branch your tmp branch, with only the changes from ./snippets
git reset --hard tmp
# push
git push --force

Just make a backup branch in case you mess something up lol

Copy link
Collaborator

@ledhed2222 ledhed2222 left a comment

Choose a reason for hiding this comment

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

see comments

natenichols and others added 9 commits October 19, 2021 13:47
* removes getFeeXrp from client
* update phrasing

* account -> address
* README: quick cleanup

Co-authored-by: Mayukha Vadari <mvadari@ripple.com>
* build: update @xrplf/typescript-style

* fix filename casing

* fix webpack setup

* add missing eslint plugin

Co-authored-by: Nathan Nichols <natenichols@cox.net>
* refactor: waits on closing test to end tests
@khancode
Copy link
Collaborator Author

Once the merge issues are resolved, feel free to merge the snippets.

Looks like there were similar changes with TransactionMetadata that Greg did in his PR that gave the impression of merge issues XRPLF/rippled#1757
Therefore, there aren't any merge issues. 🤓

@mvadari
Copy link
Collaborator

mvadari commented Oct 19, 2021

oh I see what the issue is - if you're working on top of someone else's branch, you can only use the version of develop they have, you can't merge directly from develop

@khancode khancode merged commit 5e1bc76 into mj/snippets Oct 19, 2021
@khancode khancode deleted the update-snippets branch October 19, 2021 22:07
@khancode khancode restored the update-snippets branch October 19, 2021 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues/pull requests that are primarily related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants