Skip to content

Conversation

@rakshith91
Copy link
Contributor

@rakshith91 rakshith91 commented Feb 10, 2021

Fixes #16585

Adding CloudEvent to azure-core as follows:

from azure.core.messaging import CloudEvent

Copy link
Member

Choose a reason for hiding this comment

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

  • ascii is enough, per base64 spec.
  • I'm not even sure we need to encode, could you describe the scenario where we do?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this method is confusing to me. Is the only reason for it to exist to normalize the exception type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - thinking about it, i think we dont need this method at all - removing it

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow
Warning Because naive datetime objects are treated by many datetime methods as local times, it is preferred to use aware datetimes to represent times in UTC. As such, the recommended way to create an object representing the current time in UTC is by calling datetime.now(timezone.utc).

Problem is, Python 2 doesn't have timezone.utc. @johanste usually I tell people to use msrest.serialization.utc since they use msrest as a dependency anyway, but here it won't work. Though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

I would expect an error if both data and data_base64 are present for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with it, but if we ever decide to add data_base64 in the future, this error would cause a breaking change - thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bad copying ;) - removed it

Copy link
Member

Choose a reason for hiding this comment

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

I actually find it confusing to have the body in another file here. Could we get rid of the _mock.py file and insert the dict here?

@rakshith91 rakshith91 mentioned this pull request Feb 11, 2021
Copy link
Member

Choose a reason for hiding this comment

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

1.12.0

Copy link
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

Please separate core changes and event hub changes into different PRs.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need explicit "Optional." for keyword arguments. Right? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, not sure if it's a good practice - but there can certainly be "required" keyword only params (hopefully not in our libraries, but still possible) - thought no harm in mentioning it's optional :)

Copy link
Member

Choose a reason for hiding this comment

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

Given we already pass **kwargs into cls, why we need id=event.pop("id", None), etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, intentionally popping them since we want to have the remainder of the dict as "extensions"

Copy link
Member

Choose a reason for hiding this comment

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

Please give the file a more specific name.

@xiangyan99
Copy link
Member

And you also need to bump the core dependency version to 1.12.0 for event grid. :)

@rakshith91
Copy link
Contributor Author

Please review core related chages here
Add cloud event to core by rakshith91 · Pull Request #16800 · Azure/azure-sdk-for-python (github.com)

@iscai-msft
Copy link
Contributor

/azp run python - autorest - pr

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rakshith91 rakshith91 closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move cloudevent to azure core

5 participants