Skip to content

Add filter to get all transactions methods#19

Open
JohnLZeller wants to merge 2 commits into
SynapseFI:masterfrom
JohnLZeller:zeller/expand-get-all-trans
Open

Add filter to get all transactions methods#19
JohnLZeller wants to merge 2 commits into
SynapseFI:masterfrom
JohnLZeller:zeller/expand-get-all-trans

Conversation

@JohnLZeller

@JohnLZeller JohnLZeller commented Mar 30, 2021

Copy link
Copy Markdown

The Synapse API supports using a filter string in calls to get all transactions, however, this package does not support this feature.

The API docs show this input as a string, though it's more than that, it should be a valid JSON string. This PR is the barebones approach, user is responsible for making the input a string. An alternative could obviously be to handle JSON serializing the string if it's possible, automatically under the hood. Happy to do that if you'd prefer.

jenstroeger
jenstroeger previously approved these changes Apr 27, 2021

@jenstroeger jenstroeger left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me.

@jenstroeger

jenstroeger commented Apr 27, 2021

Copy link
Copy Markdown

@JohnLZeller, thanks for the PR.

The API docs show this input as a string, though it's more than that, it should be a valid JSON string.

Either that, or a JSON-serializable object. And I guess the code would be something along those lines:

if filter is None:
    pass  # No filter, don’t serialize.
elif isinstance(filter, str):
    try:
        _ = json.loads(filter)  # All good, a valid JSON string.
    except json.decoder.JSONDecodeError as e:
        raise ValueError("filter is not a valid JSON string") from e
else:
    try:
        filter = json.dumps(filter)  # All good, a JSON serializable object.
    except TypeError as e:
        raise ValueError("filter is not JSON serializable") from e
# Continue to use `filter` as JSON serialized string.

Happy to do that if you'd prefer.

That’d be great 👍🏼

@JohnLZeller

Copy link
Copy Markdown
Author

@jenstroeger no problem, how's this! :)

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.

2 participants