-
Notifications
You must be signed in to change notification settings - Fork 1
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
[CLEANUP] Remove deprecated field 'group' and deprecated import 'from sdk' #132
Conversation
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.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> non-default parameter follows default parameter
Python things if I correctly understood how it works
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.
It's probably not worth the risk of breaking scripts of user to make it non-optional
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should raise an exception not only a warning!
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid the warning, we should avoid preemptive statements because nothings designed or planned.
What if we simply change our mind on something?
83e5bb1
to
dd6b2e1
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
outdated
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.
good spot
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.
No additional comments
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.py
following 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)