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

Milestone/0.1.3 #66

Merged
merged 7 commits into from Sep 21, 2019

Conversation

@thirtytwobits
Copy link
Contributor

commented Sep 10, 2019

Resubmitting on a branch since I don't want to upload to Pypi until after I have all the 1.3 milestone issues fixed.

Implementation of #23 Stropping and #62 Add globals
This is a complete PR for the Stropping implementation and the library
access to add globals with some other work on #64 mixed in. It is not
a 1.1.3 release candidate until #64 is complete.
Fixing tests
1. rebuilt docker to get latest py38
2. Fixing use of python keyword.
3. Making c reserved pattern more accurate.

@thirtytwobits thirtytwobits requested a review from pavel-kirienko Sep 10, 2019

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@pavel-kirienko, if you could review and approve the final change to add the implicit support through nnvg then I can merge this to the milestone branch with will not push to Pypi (I made sure only master has the Pypi upload token). This will close #23, #62, and #64 .

src/nunavut/jinja/lang/c.py Outdated Show resolved Hide resolved
src/nunavut/jinja/lang/__init__.py Outdated Show resolved Hide resolved
src/nunavut/jinja/lang/py.py Outdated Show resolved Hide resolved
@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

A quick glance at the test suite did not reveal the answer so maybe I could just ask here: what would happen if I made a DSDL namespace named pass and then generated a Python package from it?

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

A quick glance at the test suite did not reveal the answer so maybe I could just ask here: what would happen if I made a DSDL namespace named pass and then generated a Python package from it?

Erp. Missed this part. Will fix...

@thirtytwobits thirtytwobits changed the title Milestone/1.3 Milestone/0.1.3 Sep 13, 2019

@thirtytwobits thirtytwobits added this to the 1.1.3 milestone Sep 13, 2019

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

A quick glance at the test suite did not reveal the answer so maybe I could just ask here: what would happen if I made a DSDL namespace named pass and then generated a Python package from it?

Still working on this. Turned out this exposed a layering violation in Nunavut where I was only creating the concept of language support under the jinja package. I'm hoisting that up and factoring out the jinja adapters from the generic language support.

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

Okay, I think there's a big refactor needed to absorb how we've changed language support in Nunavut. I'm thinking of pulling some of the globals into a an object called LanguageContext and creating decorated versions of Generator and Namespace from this object. Something like this:

image

Thoughts?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

not bad

Refactor to support completing #64 properly.
This change moves language support up out of the jinja package. By
making language support template engine agnostic we prevent spreading
Jinja2 types throughout the namespace handling code et-al. This will
allow us to reuse the encoding and stropping logic on the namespace
generator.
@pavel-kirienko
Copy link
Member

left a comment

There are three old comments of low importance but overall it looks solid.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Should I review this now?

@thirtytwobits thirtytwobits requested a review from pavel-kirienko Sep 20, 2019

@thirtytwobits

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Okay. This (finally) fixes the stropping/encoding to work on the namespace paths. It also is setup for deeper language integration going forward. Sorry for the churn. Please review. Thanks.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Okay. Could you have a look at my earlier comments, please?

The real fix for #64
Let's try this again, shall we?

@thirtytwobits thirtytwobits force-pushed the thirtytwobits:milestone/1.3 branch from 5bcb80e to 3b5dc48 Sep 21, 2019

src/nunavut/lang/py.py Outdated Show resolved Hide resolved
src/nunavut/lang/py.py Outdated Show resolved Hide resolved
thirtytwobits and others added 2 commits Sep 21, 2019
Update src/nunavut/lang/py.py
Co-Authored-By: Pavel Kirienko <pavel.kirienko@gmail.com>
Update src/nunavut/lang/py.py
Co-Authored-By: Pavel Kirienko <pavel.kirienko@gmail.com>

@thirtytwobits thirtytwobits merged commit 69544f7 into UAVCAN:milestone/1.3 Sep 21, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.