rename API of type objects #360
Conversation
dirk-thomas
added
enhancement
dirk-thomas
force-pushed
the
definition-api-review
branch
from
5d7cbf5
to
7362648
Compare
| """ | ||
| super().__init__() | ||
| self.namespaces = namespaces | ||
| self.name = name | ||
|
|
||
| def namespaced_name(self) -> Tuple[str, ...]: | ||
| return (*self.namespaces, self.name) |
There was a problem hiding this comment.
Since this is used together in a lot of places providing the combination in a method makes sense.
There was a problem hiding this comment.
Depending on how this is used, consider making this a property, adding a little sugar.
There was a problem hiding this comment.
I would lean towards keeping it a method so the caller sees what is actually an attribute and what is derived data. If others too think it should be a property I am happy to change it though.
| """ | ||
| super().__init__() | ||
| self.namespaces = namespaces | ||
| self.name = name | ||
|
|
||
| def namespaced_name(self) -> Tuple[str, ...]: | ||
| return (*self.namespaces, self.name) |
There was a problem hiding this comment.
Depending on how this is used, consider making this a property, adding a little sugar.
dirk-thomas
force-pushed
the
definition-api-review
branch
from
7362648
to
e06cb07
Compare
|
I (force) pushed a commit with all the feedback addressed. I squashed the changes to keep them separate from the next ones updating the code to use that modified API. |
| super().__init__() | ||
| # TODO(dirk-thomas) can't be enforced yet since the parser might pass a | ||
| # constant name | ||
| # assert maximum_size > 0 |
There was a problem hiding this comment.
While I wanted to add this assert it turns out the value is currently also being used to temporary store a constant name during the parsing which gets replaced with the constant value shortly after.
I won't change this during this renaming but added a note to be aware of it.
dirk-thomas
force-pushed
the
definition-api-review
branch
from
ebab315
to
ceb4a43
Compare
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas
force-pushed
the
definition-api-review
branch
from
ceb4a43
to
a9e71c1
Compare
|
Preliminary builds passed already so this is a set of lengthy builds with all three RMW impl. enabled:
All the referenced PRs only update the naming and don't contain any other changes. I will create separate PRs to actually simplify the logic after this is merged. |
|
Please also review the changes of the second commit as well as the connected PRs which update the code base to use the renamed API. |
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
dirk-thomas
force-pushed
the
definition-api-review
branch
from
a9e71c1
to
ac4f9d5
Compare
|
As green(ish) as it gets these days (with a tint of yellow known to flaky or failing) 😉 |
The current state is not ready to be merged yet but I am looking for a review of the proposed changes. Once this is reviewed / approved I will start working on updating all message generators.
I will add some inline comments describing why I made some of the changes.