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

Migrate to Nunavut v2 #318

Merged
merged 17 commits into from
Jan 8, 2024
Merged

Conversation

Willmac16
Copy link
Contributor

Addresses #277

I'm still a bit unsure about how I'm handling nunavut_support.py, so I'd appreciate feedback on that and anything else.

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Please run Black to fix code formatting.

Also do not forget to bump the minor version when the PR is ready.

pycyphal/application/_node.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
tests/dsdl/_builtin_form.py Outdated Show resolved Hide resolved
tests/dsdl/_constants.py Outdated Show resolved Hide resolved
tests/dsdl/_manual.py Outdated Show resolved Hide resolved
pycyphal/dsdl/_compiler.py Outdated Show resolved Hide resolved
pycyphal/dsdl/_import_hook.py Outdated Show resolved Hide resolved
pycyphal/dsdl/_import_hook.py Outdated Show resolved Hide resolved
pycyphal/dsdl/_import_hook.py Outdated Show resolved Hide resolved
pycyphal/presentation/_presentation.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jan 4, 2024

Coverage Status

coverage: 94.802% (-0.4%) from 95.246%
when pulling b8f2e16 on Willmac16:Willmac16/issue277
into a6a899c on OpenCyphal:master.

@Willmac16
Copy link
Contributor Author

It looks like Yakut needs the pycyphal.dsdl.([a-z_]+) -> nunavut_support.$1 treatment.

@Willmac16
Copy link
Contributor Author

Unrelated, should the package be marked as zip_safe in setup.cfg at this point?

@pavel-kirienko
Copy link
Member

Unrelated, should the package be marked as zip_safe in setup.cfg at this point?

Maybe, but it would probably be a micro-optimization and this flag is obsolete either way, so I prefer not to touch it. See https://setuptools.pypa.io/en/latest/deprecated/zip_safe.html

@pavel-kirienko
Copy link
Member

It looks like Yakut needs the pycyphal.dsdl.([a-z_]+) -> nunavut_support.$1 treatment.

I have opened a ticket about this in the Yakut repo. Would you be available to submit the required changes there? For testing, you can replace the PyCyphal dependency specification as follows:

 install_requires =
-    pycyphal[transport-udp,transport-serial,transport-can-pythoncan] ~= 1.8
+    pycyphal[transport-udp,transport-serial,transport-can-pythoncan] ~= git+https://github.com/Willmac16/pycyphal@Willmac16/issue277

You can temporarily replace the Yakut dependency in PyCyphal integration tests in a similar fashion.

Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

It looks like Yakut needs the pycyphal.dsdl.([a-z_]+) -> nunavut_support.$1 treatment.

I have opened a ticket about this in the Yakut repo.

Wait, I have a better idea. Can we please provide wrappers in pycyphal.dsdl of the form:

def update_from_builtin(destination: T, source: Any) -> T:
    """
    A wrapper over ``nunavut_support.update_from_builtin``.
    The ``nunavut_support`` module will be generated automatically if it is not importable.
    """
    from nunavut_support import update_from_builtin

    return update_from_builtin(destination, source)

That should avoid the breakage of Yakut.


Also, could you please update the docs here:

CompositeObject and ServiceObject are simply gone now.

pycyphal/application/_port_list_publisher.py Outdated Show resolved Hide resolved
tests/presentation/_pub_sub.py Outdated Show resolved Hide resolved
@Willmac16
Copy link
Contributor Author

Willmac16 commented Jan 4, 2024

Once everything in this PR is good to go 🤞

Can I squash my commits into one to keep the commit history of upstream clean.

Edit: Looks like squashed merges are a built in GitHub feature so I'll refrain from doing it manually.

@Willmac16 Willmac16 marked this pull request as ready for review January 4, 2024 21:43
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

I think it looks good aside from a few minor nits. Please request another review when done and we'll have it merged.

docs/pages/architecture.rst Outdated Show resolved Hide resolved
pycyphal/dsdl/_compiler.py Outdated Show resolved Hide resolved
pycyphal/dsdl/__init__.py Outdated Show resolved Hide resolved
@pavel-kirienko pavel-kirienko linked an issue Jan 8, 2024 that may be closed by this pull request
@pavel-kirienko pavel-kirienko enabled auto-merge (squash) January 8, 2024 18:48
@pavel-kirienko pavel-kirienko merged commit d02f84e into OpenCyphal:master Jan 8, 2024
7 of 9 checks passed
@Willmac16 Willmac16 deleted the Willmac16/issue277 branch January 8, 2024 19:04
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.

Migrate to Nunavut v2
4 participants