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

package templates for catkin_create_pkg #1

Merged
merged 5 commits into from
Oct 16, 2012
Merged

package templates for catkin_create_pkg #1

merged 5 commits into from
Oct 16, 2012

Conversation

tkruse
Copy link
Contributor

@tkruse tkruse commented Sep 26, 2012

Added code on top of small extensions to master:

  • validate python package objects (maybe too strict?)
  • Allow keyword initialisers for package object
  • small fix about url default type

No more empy usage. Code is unit-tested and somewhat pep8 compliant

Again, I am not sure whether the generated comments in CMakeLists.txt are optimal, but I think something like this is necessary for many robotic students who do not know cmake.

A script to create packages will be added via a pull request on catkin

@tkruse
Copy link
Contributor Author

tkruse commented Sep 26, 2012

made changes to validation, fixed __init__s (changed commits), changed template marker
Require @ in email, accept upper case in packagename and starting with numbers (no warning yet, see comment).
put validate methods into classes as staticmethod, not sure whether it's good style.

@dirk-thomas
Copy link
Member

I was imagining that the validate methods are neither static nor have an argument. Wouldn't that be straight forward?

@tkruse
Copy link
Contributor Author

tkruse commented Sep 27, 2012

My thinking is that PYthon offers ducktyping, and we can use that to
validate. Meaning the validate function does not need to know internals of
the Package class (only public members) to perform the validation, and it
could do the same with any other object having the same public members,
even if that object was not an instance of class Package. If we make it a
straightforward instance method, we cannot use ducktyping any more, meaning
a programmer who wishes to validate a custom class instance of package
would have to get a Package instance from somewhere first.

In short, yes, instancemethods would be "straightforward", but not the
optimal design if we want to use pythons ducktyping feature.
It's not that important here anyway, so I can change it to what you like, I
just try to teach myself good design by comparing alternatives.

On Thu, Sep 27, 2012 at 7:50 AM, Dirk Thomas [email protected]:

I was imagining that the validate methods are neither static nor have an
argument. Wouldn't that be straight forward?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-8926508.

@dirk-thomas
Copy link
Member

Can you extract the template from the Python code into a separate file?
That would make it easier to look into the template or even copy it as a reference.

@dirk-thomas
Copy link
Member

If you could also update the template to the latest catkin state that would be great. But I can also make them after merging.

@tkruse
Copy link
Contributor Author

tkruse commented Oct 16, 2012

rebased to latest master, separated template strings into files.
Also removed generation of a CMakeLists.txt. After all, we can expect catkin macros to change between catkin versions, so each catkin version must have its own template.
Hopefully the package.xml can instead be stable or at least only changed in backwards compatible ways.

dirk-thomas added a commit that referenced this pull request Oct 16, 2012
package templates for catkin_create_pkg
@dirk-thomas dirk-thomas merged commit 015e808 into ros-infrastructure:master Oct 16, 2012
@dirk-thomas
Copy link
Member

Please re-add the CMakeLists template (even if it might need changes in the future). But we need a working version of that script asap.

@tkruse
Copy link
Contributor Author

tkruse commented Oct 16, 2012

I already re-added the CMakeLists.txt to branch templates in catkin. You
closed the pull request earlier:
ros/catkin#173
I cannot reopen it myself, it seems.

If we put anything catkin-version dependent into catkin_pkg, we will regret
it later.
Therefore I try to only put the generation of package.xml into catkin_pkg,
and anything else into catkin.

On Wed, Oct 17, 2012 at 1:12 AM, Dirk Thomas [email protected]:

Please re-add the CMakeLists template (even if it might need changes in
the future). But we need a working version of that script asap.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-9510568.

dirk-thomas pushed a commit that referenced this pull request Apr 4, 2018
* `parse_package_string()` raises `InvalidPackage` exception with the package name in the message.
This work is needed for [https://github.com/ros-infrastructure/rosdep/issues/598](https://github.com/ros-infrastructure/rosdep/issues/598)

* Add filename to the InvalidPackage exception coming from `Package.validate()`

* `InvalidPackage` has `msg` and`package_path` (#1)

`InvalidPackage` exception has `msg`, `package_path` and defines `__str__()`

Note that `parse_package_string()` catches `InvalidPackage` exception and sets
the filename; it is needed because the functions that called in
`_parse_package_string()` raise InvalidPackage exception without filename,
e.g. `_get_node()`

* Minor code review fixes
- Use double quotes for string literals with single quotes inside
- Use forward slash instead of backward slash in file path

* Pass filename to functions that raise InvalidPackage

Remove not used `_get_optional_node_value()`
Remove try/except block that was adding filename to InvalidPackage
exception

* nipick: improve error message
dirk-thomas pushed a commit that referenced this pull request May 3, 2018
* Enable flake8 for the project as a unit test. The test is copied from [https://github.com/ros-infrastructure/rosdep/blob/d31c5320b95f4e9ff6a0aad56a8531cb138fc079/test/test_flake8.py](https://github.com/ros-infrastructure/rosdep/blob/d31c5320b95f4e9ff6a0aad56a8531cb138fc079/test/test_flake8.py)
Code re-formatted, fixed other issues raised by flake8

* Do not run flake8 test on Python 3.2
Add Python 3.6 to Travis CI

* keep operator trailing

* avoid aligned indentation

* avoid aligned indentation

* remove unnecessary parenthesis

* avoid aligned indentation

* remove old comment

* conf.py doesn't exist

* remove usage of format_map which is not available in Python 2.7

* remove usage of format_map which is not available in Python 2.7

* Include package name in error message (#212)

* Include package name in error message

* Wrapped package name in quotes for consistency

* Add support for file attribution of license tag (#202)

* InvalidPackage with package path (#209)

* `parse_package_string()` raises `InvalidPackage` exception with the package name in the message.
This work is needed for [https://github.com/ros-infrastructure/rosdep/issues/598](https://github.com/ros-infrastructure/rosdep/issues/598)

* Add filename to the InvalidPackage exception coming from `Package.validate()`

* `InvalidPackage` has `msg` and`package_path` (#1)

`InvalidPackage` exception has `msg`, `package_path` and defines `__str__()`

Note that `parse_package_string()` catches `InvalidPackage` exception and sets
the filename; it is needed because the functions that called in
`_parse_package_string()` raise InvalidPackage exception without filename,
e.g. `_get_node()`

* Minor code review fixes
- Use double quotes for string literals with single quotes inside
- Use forward slash instead of backward slash in file path

* Pass filename to functions that raise InvalidPackage

Remove not used `_get_optional_node_value()`
Remove try/except block that was adding filename to InvalidPackage
exception

* nipick: improve error message

* Fix flake8 issues (1 empty line instead of 2) for recently updated code
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.

2 participants