Skip to content

Add AddAptRepo class with spec#14

Open
andis wants to merge 1 commit intosometimesfood:masterfrom
andis:contrib/add-apt-repo
Open

Add AddAptRepo class with spec#14
andis wants to merge 1 commit intosometimesfood:masterfrom
andis:contrib/add-apt-repo

Conversation

@andis
Copy link
Contributor

@andis andis commented Jun 16, 2015

This pr adds a contrib class AddAptRepo.

@andis andis force-pushed the contrib/add-apt-repo branch from 97522c0 to 40a3432 Compare June 16, 2015 10:27
@sometimesfood
Copy link
Owner

Thanks for the pull request, this looks great.

However, there's a couple of small things I would like to see changed before I merge this:

  • bundle exec rubocop shows a number of warnings. Could you please fix them?
  • Please remove the core_file and core_kernel member variables and use fakefs and a simple Kernel.stub in the tests instead.
  • The attr_reader for wright should be private.

The documentation is not parsed correctly by yard and has a few other issues:

  • The @return tag simply renders as 'self' which is probably not what you wanted. In this case I would simply specify [void] as a return value since the actual return value should not be part of the public API here.
  • The class itself does not have a return value. Please move the @params, @return etc. to #call or #initialize and keep only the short explanation and the usage examples in the class comment.
  • The examples should have two separate @example tags so they are rendered independently. Also, could you add a #! line so that the context from which self can be passed as the 'wright' argument is clear?
  • The type annotations for the parameters are missing.
  • The example needs to be indented by two spaces. The options in the option hash should be documented using the @option tag.

Just run bundle exec yard and have a look at the resulting docs and you'll see where the generated documentation is falling short of your expectations.

@sometimesfood
Copy link
Owner

Also, can you please make sure that all of your code is run at least once in the tests?

The coverage report (COVERAGE=1 bundle exec rake test) shows that there are some lines that are not covered by the specs.

@andis
Copy link
Contributor Author

andis commented Jun 23, 2015

Work in progress (docs):

  • @return tag
  • move @params, @return etc. to #call or #initialize
  • @example tags
  • type annotations for the parameters
  • indentation
  • option hash

@andis andis force-pushed the contrib/add-apt-repo branch 2 times, most recently from f4e477f to 4411407 Compare June 30, 2015 09:30
@andis
Copy link
Contributor Author

andis commented Jun 30, 2015

  • fix rubocop warnings
  • remove core_file
  • remove core_kernel
  • increase test coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants