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

Refactoring and pydoc comments #38

Closed
wants to merge 10 commits into from
Closed

Conversation

Guelakais
Copy link

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

@esteve
Copy link
Contributor

esteve commented Aug 19, 2024

@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
Copy link
Author

@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
Copy link
Member

luca-della-vedova commented Aug 20, 2024

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 is really just reading the function body line by line and doesn't add anything.
  • This same as the above
  • This Same again
  • This 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.

@Guelakais
Copy link
Author

Guelakais commented Aug 20, 2024

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
Copy link
Contributor

esteve commented Aug 20, 2024

@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.

@Guelakais Guelakais closed this Aug 20, 2024
@Guelakais Guelakais deleted the refactoring branch August 20, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants