[CLEANUP] Remove deprecated field 'group' and deprecated import 'from sdk' #132
Conversation
Mildophin
requested review from
nmenetrier,
MatthieuMoreau0,
oliver-gordon and
Augustinio
Mildophin
changed the title
Mildophin
changed the title
pasqal_cloud/__init__.py
Outdated
| def __init__( | ||
| self, | ||
| project_id: str, | ||
| username: Optional[str] = None, | ||
| password: Optional[str] = None, | ||
| token_provider: Optional[TokenProvider] = None, | ||
| endpoints: Optional[Endpoints] = None, | ||
| auth0: Optional[Auth0Conf] = None, | ||
| webhook: Optional[str] = None, |
There was a problem hiding this comment.
mention this in the changelog since a common way before of instantiating the sdk would have been
sdk = SDK("awennersteen", "super-secret", project_id="my-project_id")And this won't work anymore.
There was a problem hiding this comment.
Indeed, bad python practices do not help haha, I will update the changelog for this potential breaking change.
There was a problem hiding this comment.
I'm not sure I understand why you decided to move the project_id to the first argument
There was a problem hiding this comment.
-> non-default parameter follows default parameter
Python things if I correctly understood how it works
There was a problem hiding this comment.
It's probably not worth the risk of breaking scripts of user to make it non-optional
There was a problem hiding this comment.
If this is going to cause significant breaks, we could accommodate by still defaulting to None and keep on raising an exception in the scenario it's missing.
We have optional input for several fields that are very obviously not optional fields. Can't authenticate without a password.
There was a problem hiding this comment.
Then I will put it back as optional and add a warning if not given. Thx for the feedback.
There was a problem hiding this comment.
You should raise an exception not only a warning!
There was a problem hiding this comment.
I meant, an exception for sure, but also a warning saying that project_id will be the first argument later, like a deprecation warning.
There was a problem hiding this comment.
I'd avoid the warning, we should avoid preemptive statements because nothings designed or planned.
What if we simply change our mind on something?
Mildophin
force-pushed
the
mv/remove-group
branch
from
83e5bb1
to
dd6b2e1
Compare
Mildophin
force-pushed
the
mv/remove-group
branch
from
dd6b2e1
to
9b8fec9
Compare
CHANGELOG.md
Outdated
| ### Breaking change | ||
|
|
||
| - 'from pasqal_cloud' import has completely replaced the deprecated import 'from sdk' | ||
| - 'project' field has completely replaced the deprecated field 'group' |
There was a problem hiding this comment.
field is project_id and this MR is simply removing group_id, We introduced project_id a while ago
There was a problem hiding this comment.
Should I write "'group' field is now removed, use 'project' field instead" ? Not sure how to make the message clear when we read the sentence.
CHANGELOG.md
Outdated
|
|
||
| - 'from pasqal_cloud' import has completely replaced the deprecated import 'from sdk' | ||
| - 'project' field has completely replaced the deprecated field 'group' | ||
| - Now you need to declare the 'project_id' as the first argument when instantiating an SDK class |
Description
Simple cleanup PR that removed the deprecated field 'group', making compulsory the use of 'project' instead.
Also it removes the old way of importing this sdk with 'from sdk', it's been deprecated for more than year now and it is removed in this PR. We have to use 'from pasqal_cloud' from now.
Checklist
Versioning (PASQAL developers only)
_version.pyfollowing the changes in your PR and by using semantic versioning.Documentation
Tests
Internal tests pipeline (PASQAL developers only)
If your PR hasn't changed any functionality, it still needs to be validated against internal tests.
After updating the version (PASQAL developers only)