Skip to content
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

Document changes to sensor value coercion #99

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Conversation

bmerry
Copy link
Contributor

@bmerry bmerry commented Sep 3, 2024

No description provided.

@coveralls
Copy link

Coverage Status

coverage: 92.721%. remained the same
when pulling 8da807e on doc-sensor-type-force
into b6d34df on master.

Copy link
Contributor

@amishatishpatel amishatishpatel left a comment

Choose a reason for hiding this comment

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

One suggestion on your reference to core.Address. Otherwise, perfect.

==================== ==========================
:class:`int` :class:`numbers.Integral`
:class:`float` :class:`numbers.Real`
:class:`~.Timestamp` :class:`numbers.Real`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great that ~.Timestamp just knows to go specifically to aiokatcp.core.

Comment on lines +18 to +19
- The :class:`Address` constructor now raises :exc:`TypeError`
if the first argument is not an IP address object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's unlikely that the first argument to core.Address will change from being host. Still, is it worth explicitly stating which arg it is?

  • e.g. "[...] if the ':attr:host' argument is not an IP address object." (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, six of one, half a dozen of the other.

Comment on lines +8 to +11
- When setting the value of a sensor, it is coerced to
an instance of the sensor type (see :ref:`sensor-value-coercion`). Reading
back the sensor value will give this coerced object instead of the original.
This only applies if the value was not already of the expected type.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is about as perfectly succinct as it gets. I've written and deleted my suggestions a few times; anything I think of makes it more convoluted/pedantic.

Comment on lines +21 to +23
Other changes:

- Use :func:`typing.get_args` instead of an undocumented API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Genuine question, why is it necessary to indicate this? I see the original change was from b3d0f7c

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 considered not mentioning it does it doesn't affect how users use the package, but it is a change that users might want to know about if some future Python version removes the undocumented API and the old version stops working.

@bmerry bmerry merged commit 081885d into master Sep 3, 2024
21 of 22 checks passed
@bmerry bmerry deleted the doc-sensor-type-force branch September 3, 2024 15:31
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