Skip to content

Conversation

@rjrudin
Copy link
Contributor

@rjrudin rjrudin commented Oct 9, 2023

Will tackle eval in a separate PR, had a lot to add with this one.

Will tackle eval in a separate PR, had a lot to add with this one.
| `xml` | XML document defining the columns and rows. |
| `csv` | CSV text with the first row defining the columns. |
| `json-seq` | A [line-delimited JSON sequence](https://datatracker.ietf.org/doc/html/rfc7464) with the first row defining the columns. |
| `mixed` | TODO Seems like we should remove this as it does the same thing as "return_response". |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BillFarber See my note here - thoughts on removing this? I doubt it would ever be used, and return_response=True would be the more consistent way to just get the original multipart/mixed response back.

Copy link
Contributor

Choose a reason for hiding this comment

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

If 'mixed' just has the same affect as "return_response=true", then I would remove it.
However, would it work and be useful to use Client.process_multipart_mixed_response(response) to pre-process requests with the 'mixed' format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know of yet, and in fact, one reason I'd like to move the eval stuff out of client.py is so that the process_multipart... method isn't yet public. While it may be useful in the future as a public method, we don't know that yet, and I lean towards making things non-public when in doubt (as opposed to what I did with e.g. ml-app-deployer).

Copy link
Contributor

@BillFarber BillFarber left a comment

Choose a reason for hiding this comment

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

It's fine with me if you want to remove 'mixed' first.

@rjrudin
Copy link
Contributor Author

rjrudin commented Oct 10, 2023

It's fine with me if you want to remove 'mixed' first.

Will keep it here, and likely remove in a separate PR.

@rjrudin rjrudin merged commit f3ecca1 into develop Oct 10, 2023
@rjrudin rjrudin deleted the feature/589-docs branch October 10, 2023 11:52
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.

3 participants