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

Issue #299: Types provided for effects, with Go Package being the ref… #635

Merged
merged 6 commits into from Apr 26, 2021

Conversation

RabeaWahab
Copy link
Contributor

@RabeaWahab RabeaWahab commented Apr 21, 2021

  • Types provided for effects, with Go Package being the reference.
  • Moving Asset, Trade and Offer Types to their own files.
  • Kept service_api exporting back types imported to not break anything in other files.

Any feedback is really appreciated.

Closes #299.

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Thank you for this work; it's a great contribution! I just noticed some minor typos in the type definitions; please give those a look.

src/server_api.ts Outdated Show resolved Hide resolved
src/server_api.ts Outdated Show resolved Hide resolved
src/server_api.ts Outdated Show resolved Hide resolved
src/server_api.ts Outdated Show resolved Hide resolved
src/server_api.ts Outdated Show resolved Hide resolved
src/server_api.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Awesome, looks great now! As a final touch, can you add a note (under the Unreleased heading) to the CHANGELOG summarizing this? Please follow the format of the other entries.

src/types/effects.ts Outdated Show resolved Hide resolved
…nes + removed a type that was a duplicate + CCHANGELOG entry
@RabeaWahab RabeaWahab changed the title Issue #299: Types peovided for effects, with Go Package being the ref… Issue #299: Types provided for effects, with Go Package being the ref… Apr 26, 2021
@Shaptic
Copy link
Contributor

Shaptic commented Apr 26, 2021

Last hurdle, @RabeaWahab, as you can see in the approval checklist: all of your commits need to be GPG signed! It's a bit of effort to set up, but our repository unconditionally requires that commits be signed.

Once you've configured your keys, since you already have unsigned commits, you will need to rebase & squash them all into a single, signed commit, e.g.

git rebase -i HEAD~6

The first line should be the commit right before your PR's commits. Edit the lines to say f instead of pick for all but the first commit, then save+exit to finish the rebase, and finally,

git commit --amend -S
git push -f

to sign it.

@Shaptic
Copy link
Contributor

Shaptic commented Apr 26, 2021

Actually, I'm going to go ahead and merge this. Be sure to set up signed commits for the future, though, this project and many others impose these kinds of requirements to trace changes and provide author authenticity.

Thank you again for your contribution! This will be part of the next release. 👍

@Shaptic Shaptic merged commit e6c622e into stellar:master Apr 26, 2021
@RabeaWahab
Copy link
Contributor Author

Thank you @Shaptic , I will have it ready for the next PR. 🙏 again I appreciate the quick feedback from you and @paulbellamy

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.

Granular type definitions for effects
3 participants