Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Simulators API #2006
base: master
Are you sure you want to change the base?
Simulators API #2006
Changes from 104 commits
f42cc6c
8e936c2
67f3680
2852e9f
4fb7728
8a96799
f061996
2a67cb5
966f31d
947c0c2
f25b46d
741c6a0
7c5fc0f
3460781
7d5e13c
4061ed2
ef47c43
7de0581
bd058e4
0fd679d
76d3c51
a70bab1
74ae117
e041dad
c940dbc
5a9f01e
079075b
27fe6bd
8e7ae96
2001b37
3a5a139
75c6189
334bfad
281a669
5947ad7
b6672ff
5d09fb0
016227c
7bd1684
a3d3a04
e01cec3
e9bde70
47d33a7
985e1ff
ce5c8f1
0a37df1
5fff3a4
6b82751
774252b
8991a38
e4d437e
00e56c6
5342df5
70833ab
2a6ae37
aa7702b
3473533
2400a67
78d2d79
cd40c3b
5e4e0af
f0139d0
bb51ef6
2fb1eef
b668282
c8527a9
9798475
c076dc9
77d431b
e43ce27
f49d9fa
b683f02
b1b5c9d
7084a4d
4d5fa3b
dbbe91a
9520e8a
66f4074
655d298
92b3d9a
c316ce3
560188b
7b7d82b
5a77f2e
fd3342b
d7236a8
64cd8b5
2a06f98
6e795d6
3560687
48d6195
eb024db
c75f492
c104a3b
6718fb4
960775a
6bdc7f3
1cdc8b2
9fead1a
5d125bd
d15226f
eef86be
bb04cca
a48b626
c76eab1
d5600fb
f12fb21
60c83a0
4898f54
9945ff8
c8180cb
16fe676
cd4498c
1c85b88
7def138
49ad5fc
6e4f8bb
0a9d007
68bf6a8
d79b8ff
daf3e8a
f614c49
88446b4
6b31e78
ebfe644
433e585
9428ecb
7401368
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this will auto paginate if limit is set to
None
,-1
orfloat("inf")
, following the normal Python SDK patternThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost agree with @evertoncolling comment above 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need one example showing where to import
SimulationRunsFilter
and how to use it (can include thesort
in the same example)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as this follows the API design guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list class needs to be renamed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add something like this:
Then we don't need this to take up 5 lines:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@polomani since the maximum number of integrations a project can have is 20, should we lower the default max limit to 20 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evertoncolling this part
Defaults to 1000.
is not correct here, since the method applieslimit: int = DEFAULT_LIMIT_READ
, which is 25.so it's not the same as API could potentially give and seems like SDK default it quite nice (so the output readable for the user by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I was not aware the SDK limit had a different value. Sounds good then, but we need to update the description to account the SDK limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an example showcasing the usage of filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See prev. comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use
retrieve_multiple
anymore, it is a remnant from the "dark ages" when we didn't havetyping.overload
to indicate the correct return type depended on the given inputs.Check out e.g.
UserProfilesAPI.retrieve
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for this to chunk according to the API limits (100, right?), you need to set:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so there is an example of "retrieve_multiple", but there is no test, does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an example showcasing filter & sort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also extend this to support multiple. It seems the API only returns 1 at the time, so you would need to set
self._RETRIEVE_LIMIT = 1
in__init__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using numbers when illustrating usage of external ID 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.