Skip to content
This repository was archived by the owner on Jan 13, 2023. It is now read-only.

Conversation

@pdecol
Copy link
Contributor

@pdecol pdecol commented Sep 21, 2019

resolves #235

@pdecol
Copy link
Contributor Author

pdecol commented Sep 21, 2019

I usually like it to clean up code that I touch. Since making the code PEP8 conform is still work in progress I changed the indentation level to 4 spaces in existing test files where I added more tests. That is the reason why this pull request has quite a lot of line changes. If its preferred to have these changes in a separate ticket/pull request let me know and I can change it back to 2 spaces.

'balances': f.Array | f.FilterRepeater(f.Int),

'milestone':
f.ByteString(encoding='ascii') | Trytes(Address),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is a potential backwards incompatible change. milestone was removed in IRI version 1.4.1.6 (https://github.com/iotaledger/iri/blob/v1.4.1.6/src/main/java/com/iota/iri/service/dto/GetBalancesResponse.java) which was released on the 6th of January 2018. Since this was more than one and a half years ago I think it is justified to remove it here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Sounds good to me.

@todofixthis
Copy link
Contributor

Hi @pdecol thanks for working on this! Really appreciate the attention to detail and being proactive about keeping the codebase consistent!

Will have a look at this today 👍🏻

Copy link
Collaborator

@lzpap lzpap left a comment

Choose a reason for hiding this comment

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

amazing, thank you for your contribution :)

Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Minor changes requested, but generally looks really good, thanks @pdecol!

@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

'balances': f.Array | f.FilterRepeater(f.Int),

'milestone':
f.ByteString(encoding='ascii') | Trytes(Address),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Sounds good to me.

@lzpap
Copy link
Collaborator

lzpap commented Sep 23, 2019

Minor changes requested, but generally looks really good, thanks @pdecol!

@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

Hey @todofixthis I will start with coordinating with the other libs to see if they validate requests/responses. I am generally for filters in both directions. Then you could handle exceptions and also it might be easier to reveal bugs with future IRI API changes.

@lzpap
Copy link
Collaborator

lzpap commented Sep 24, 2019

Minor changes requested, but generally looks really good, thanks @pdecol!
@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

Hey @todofixthis I will start with coordinating with the other libs to see if they validate requests/responses. I am generally for filters in both directions. Then you could handle exceptions and also it might be easier to reveal bugs with future IRI API changes.

@todofixthis I talked with the devs of the other libraries, and the conclusion is that responses are not filtered/validated/checked in general, except bundle and transactions hashes. I think it would be nice to make PyOTA consistent with this behavior.

@pdecol pdecol requested a review from todofixthis September 28, 2019 17:19
@pdecol
Copy link
Contributor Author

pdecol commented Sep 28, 2019

Thanks for the review! I implemented the requested changes and reverted the changes to the response filters for the fields that we decided not to validate.

Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Two more changes requested; apologies for not bringing them up during the first review. Once these last two items are addressed, this is good to go!

@pdecol
Copy link
Contributor Author

pdecol commented Sep 29, 2019

No worries. Thanks for spotting this mistake. I removed f.Required and f.Unicode and I added f.ByteString so that it is consistent with the response filters in findTransaction, getTrytes, getTips and getMissingTransactions.

One more thing that I would like to ask you. Can you change this URL here in GitHub? (https://iota.readme.io/ -> https://docs.iota.org/) I think this should be the very last URL that needs to be adjusted.

image

@todofixthis
Copy link
Contributor

LGTM Thanks @pdecol !!

@lzpap
Copy link
Collaborator

lzpap commented Sep 30, 2019

Thanks @pdecol !

@lzpap lzpap merged commit 7617d2b into iotaledger-archive:develop Sep 30, 2019
@pdecol pdecol deleted the feature/235-update-core-api branch October 10, 2019 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants