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

Added @return doc comment annotations to methods in azure_client.rb. #655

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

Justin-W
Copy link
Contributor

@Justin-W Justin-W commented Jan 12, 2022

Background:

There were a few utility methods in azure_client.rb called by code related to PR #651 which had no doc comments. And some of those methods' implementations had changed over time so that their method names had become inaccurate. While adding return type annotations to those methods, I noticed many other methods in the same file which also lacked any doc comments and had names which weren't drectly indicative of the type of the value(s) returned, but which weren't directly related to #651.

Since there were also many other un-annotated methods within azure_client.rb which also had names which were non-indicative of the return type (thus requiring a maintainer to read each method's implementations to understand what type of value would be returned), I opportunistically added @return annotations to many of those methods, assuming that such annotations would be better than completely un-annotated methods. I am submitting these additional annotations as a separate PR since they weren't directly related to the functionality in #651.

Checklist:

Please check each of the boxes below for which you have completed the corresponding task:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [N/A] I have added tests that prove my fix is effective or that my feature works
  • All unit tests pass locally (after my changes)
  • Rubocop reports zero offenses (after my changes)

Please include below the summary portions of the output from the following 2 scripts:

pushd src/bosh_azure_cpi
  ./bin/test-unit
  ./bin/rubocop_check
popd

NOTE: Please see how to setup dev environment and run unit tests in docs/development.md.

Unit Test output:

972 examples, 0 failures
Coverage report generated for RSpec to /Users/justinw/go/src/github.com/cloudfoundry/bosh-azure-cpi-release/src/bosh_azure_cpi/coverage. 4182 / 4203 LOC (99.5%) covered.

Rubocop output:

184 files inspected, no offenses detected

Changelog

  • Added @return doc comment annotations to methods in azure_client.rb.

Background:

There were a few utility methods in `azure_client.rb` called by code related to PR cloudfoundry#651 which had no doc comments. And some of those methods' implementations had changed over time so that their method names had become inaccurate. While adding return type annotations to those methods, I noticed many other methods in the same file which also lacked any doc comments and had names which weren't drectly indicative of the type of the value(s) returned, but which weren't directly related to cloudfoundry#651.

Since there were also many other un-annotated methods within `azure_client.rb` which also had names which were non-indicative of the return type (thus requiring a maintainer to read each method's implementations to understand what type of value would be returned), I opportunistically added `@return` annotations to many of those methods, assuming that such annotations would be better than completely un-annotated methods. I am submitting these additional annotations as a separate PR since they weren't directly related to the functionality in cloudfoundry#651.
@rkoster rkoster requested review from a team, jpalermo and lnguyen and removed request for a team January 13, 2022 15:45
Copy link
Member

@jpalermo jpalermo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The # @return [Hash, nil] makes me think that the function returns an array of Hash and nil, rather than expressing that it returns either.

But that's just something to think about, and maybe it's just me. I will add though, that the whole @return pattern in this file adds virtually zero value to the file in the current form.

Api documentation can be good when it is correct and helpful. But being told that a method returns Hash, Array, or Integer doesn't really provide any value.

If I'm given a function with no docs, I have to look at it, and see that it's returning a hash, and what keys are in that hash.

If I'm told that it returns Hash, I still have to look at it and see what keys it returns. Being given the Hash hint hasn't really saved me any meaningful time.

But this commit isn't introducing the pattern, just cleaning up some rough edges in it. Thanks for doing that! 👍

@rkoster
Copy link
Contributor

rkoster commented Jan 20, 2022

Thanks @Justin-W

@rkoster rkoster merged commit 3c17992 into cloudfoundry:master Jan 20, 2022
@Justin-W Justin-W deleted the misc-1 branch January 26, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants