colcon / colcon-cargo

An extension for colcon-core to support Rust projects built with Cargo
http://colcon.readthedocs.io
Apache License 2.0
30 stars 20 forks source link

Refactoring and pydoc comments #38

Closed Guelakais closed 2 weeks ago

Guelakais commented 2 months ago

made some refactorings to make the code a little bit cleaner. Mosly I've added comments.

esteve commented 2 weeks ago

@Guelakais could you undo all the code changes and only leave the comments? Your code changes are making the lint checker fail (see https://github.com/colcon/colcon-cargo/actions/runs/9723715491/job/27488484940?pr=38)

Guelakais commented 2 weeks ago

@Guelakais could you undo all the code changes and only leave the comments? Your code changes are making the lint checker fail (see https://github.com/colcon/colcon-cargo/actions/runs/9723715491/job/27488484940?pr=38)

yep

luca-della-vedova commented 2 weeks ago

I am personally not in favor of this kind of changes, the docstrings are very verbose and in most cases don't really add any information. In my opinion a docstring should document the purpose of the function, maybe a brief summary and any additional information that is not obvious from looking at the code, this is not really the case for most (any?) of the changes here.

For example:

I would advise against adding a docstring to every single function just because public functions should have documentation. Instead I would only add docstrings to more complex functions to document what is not obvious by looking at the code.

Guelakais commented 2 weeks ago

I am personally not in favor of this kind of changes, the docstrings are very verbose and in most cases don't really add any information. In my opinion a docstring should document the purpose of the function, maybe a brief summary and any additional information that is not obvious from looking at the code, this is not really the case for most (any?) of the changes here.

For example:

* [This](https://github.com/colcon/colcon-cargo/pull/38/files#diff-4dfc267f19910cce49f026937811aeed0356428ad09f827eaa47f79d99744407R18-R23) is really just reading the function body line by line and doesn't add anything.

* [This](https://github.com/colcon/colcon-cargo/pull/38/files#diff-ded1e9d012400d7b2eb7d1ad758a86e7f94e8d7a2737196a5c3f45acabd2b633R22-R27) same as the above

* [This](https://github.com/colcon/colcon-cargo/pull/38/files#diff-31e195faddd33844025925697fd752815340dbc89eaf1f8b122c7175fd23af3dR21-R25) Same again

* [This](https://github.com/colcon/colcon-cargo/pull/38/files#diff-31e195faddd33844025925697fd752815340dbc89eaf1f8b122c7175fd23af3dR30-R37) a 7 line comment for a function that has a body of `pass`, and it doesn't even say _why_ it is implemented but is a noop (which is really the only information someone who reads that function would need).

I would advise against adding a docstring to every single function just because public functions should have documentation. Instead I would only add docstrings to more complex functions to document what is not obvious by looking at the code.

In exactly the same way, you could simply remove the methods that do nothing. In case of doubt, it is not obvious exactly what a method does, as neither the input nor the output type are explicitly described. You can also save yourself a lot of documentation if you simply write the corresponding data type after each input parameter and the output type at the end of the method with -> type. This was not done. In my opinion, the current version of the code does not describe itself sufficiently. Therefore -> comments. Whether you use docstrings or all comments with // or # is ultimately irrelevant as long as everyone understands what a method does.

I've just been looking for a good example of what I mean. The following method doesn't really need any additional comments, as it's perfectly clear what's going on. It is clear at all times what goes into the method and what comes out of the method. It's Python, by the way.

def create_point(x_axe:float,y_axe:float,z_axe:float) -> Point:
    point=Point()
    point.x=x_axe
    point.y=y_axe
    point.z=z_axe
    return point
esteve commented 2 weeks ago

@Guelakais the problem is that many of the comments you've added don't describe the intent of the method, but just feel like they are a line by line read of the code, for example:

https://github.com/colcon/colcon-cargo/pull/38/files#diff-4dfc267f19910cce49f026937811aeed0356428ad09f827eaa47f79d99744407R18-R23

How does this comment improve readability? It does is just describe each line of code, but nothing else.