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

[docs] YARD: avoid warnings and check the docs #491

Merged

Conversation

olleolleolle
Copy link
Contributor

This PR improves the YARD documentation annotations.

  • fix some typos in directives
  • fix some copy and paste errors in names of things
  • avoid using p as a local variable name

Found all these using the yard-junk gem.

  - avoid using p as a local variable name
Copy link
Member

@maxmeyer maxmeyer left a comment

Choose a reason for hiding this comment

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

Are you aware of a tool or have experience using such a tool to test our documentation?

Does it makes sense to add such a tool to our test suite or at least have an icon in our README? What do you think. Feel free to have a look at some helpful tools and add them to the toolchain. Though I'm not sure, if those tools should make our test suite fail - without a failure adding them to the test suite does not make sense.

@@ -234,13 +234,13 @@ def run_command(*args)
# @param [Hash] options
# Options for aruba
#
# @option [TrueClass,FalseClass] fail_on_error
# @option options [TrueClass,FalseClass] fail_on_error
Copy link
Member

Choose a reason for hiding this comment

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

Does it makes sense to have Boolean here as well?

Copy link
Member

Choose a reason for hiding this comment

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

There are many more files. Would you like to change it yourself? And change all occurences of [TrueClass, FalseClass] to [Boolean]

  • lib/aruba/api/command.rb
  • lib/aruba/api/deprecated.rb
  • lib/aruba/matchers/command/be_successfully_executed.rb
  • lib/aruba/matchers/command/have_exit_status.rb
  • lib/aruba/matchers/command/have_finished_in_time.rb
  • lib/aruba/matchers/command/have_output.rb
  • lib/aruba/matchers/command/have_output_on_stderr.rb
  • lib/aruba/matchers/command/have_output_on_stdout.rb
  • lib/aruba/matchers/command/have_output_size.rb
  • lib/aruba/matchers/directory/be_an_existing_directory.rb
  • lib/aruba/matchers/directory/have_sub_directory.rb
  • lib/aruba/matchers/file/be_a_command_found_in_path.rb
  • lib/aruba/matchers/file/be_an_existing_executable.rb
  • lib/aruba/matchers/file/be_an_existing_file.rb
  • lib/aruba/matchers/file/have_file_content.rb
  • lib/aruba/matchers/file/have_file_size.rb
  • lib/aruba/matchers/file/have_same_file_content.rb
  • lib/aruba/matchers/path/a_path_matching_pattern.rb
  • lib/aruba/matchers/path/be_an_absolute_path.rb
  • lib/aruba/matchers/path/be_an_existing_path.rb
  • lib/aruba/matchers/path/have_permissions.rb
  • lib/aruba/matchers/path/match_path_pattern.rb
  • lib/aruba/matchers/rspec_matcher_include_regexp.rb
  • lib/aruba/platforms/aruba_file_creator.rb
  • lib/aruba/platforms/aruba_fixed_size_file_creator.rb
  • lib/aruba/platforms/unix_platform.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the replacement.

Copy link
Member

@maxmeyer maxmeyer Sep 5, 2017

Choose a reason for hiding this comment

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

You missed one in lib/aruba/api/command.rb. It's without an " " in between.

@olleolleolle
Copy link
Contributor Author

Stating the obvious: A CI tool is good as long as users act on the outputs created. If not, it's no use. At the moment, I don't feel it would make things much better quickly.

The benefit of yard-junk is its improvements (clarification, grouping) of the YARD output.

In short: I have no strong opinion on this issue.

@olleolleolle olleolleolle force-pushed the fix/yard-documentation-detail branch from b3d0ac7 to 47517c7 Compare September 5, 2017 20:08
@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

Ok. But I helps to have this tool installed in an developer environment to check the docs if she/he feels it's required? Let's add a "Rake" task for that and add the gem to the "development" group in our Gemfile.

@olleolleolle
Copy link
Contributor Author

@maxmeyer yard-junk ships with a rake task like that.

@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

@olleolleolle Great!

@olleolleolle
Copy link
Contributor Author

And, if you ever need to add a true or false or nil to a YARD Type list, you can:
http://www.rubydoc.info/gems/yard/file/docs/Tags.md#Literals

@olleolleolle olleolleolle changed the title YARD: avoid yard warnings [docs] YARD: avoid warnings Sep 5, 2017
@olleolleolle
Copy link
Contributor Author

olleolleolle commented Sep 5, 2017

@maxmeyer Would you prefer if I added the yard-junk tool as a development dependency in the Gemfile in this PR, or as a separate change?

@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

I think it is ok to add it in this PR in a separate commit.

@maxmeyer maxmeyer changed the title [docs] YARD: avoid warnings [docs] YARD: avoid warnings and check the docs Sep 5, 2017
@olleolleolle
Copy link
Contributor Author

olleolleolle commented Sep 5, 2017

@maxmeyer Maybe this is a deal-breaker: requires 2.3.0+

https://github.com/zverok/yard-junk/blob/master/yard-junk.gemspec#L14

@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

No problem, we use the RUBY_VERSION at some places. I think using it "again", is not a problem from my side. And it's only tooling.

Gemfile:

  gem 'yard', '~>0.9.9'

  if RUBY_VERSION > '2.3'
    gem 'yard-junk', '~>0.0.2'
  end

Be careful, the documentation in the README is not correct. It's .define_task, not .task.

Rakefile:

if RUBY_VERSION > '2.3'
  require 'yard-junk/rake'
  YardJunk::Rake.define_task
end

@olleolleolle
Copy link
Contributor Author

Yes, it looks neat.

screenshot 2017-09-05 22 27 04

@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

I think the addition is definitively helpful. 😄 Thanks for raising this issue!

And adding it to CI (in the future) would ensure some kind of a standard + ensures better quality of our documentation which would help our users + us as developers.

Rakefile Outdated
require 'yard-junk/rake'
YardJunk::Rake.define_task
rescue LoadError # rubocop:disable Lint/HandleExceptions
# Not an issue; this lint tool requires Ruby 2.3+
Copy link
Member

@maxmeyer maxmeyer Sep 5, 2017

Choose a reason for hiding this comment

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

I don't like "rescue" lib loading without any action. This might result in a search for a failure. Please output at least a warning. Which then can be found on CI - if we choose to add it to the test pipeline.

Copy link
Contributor Author

@olleolleolle olleolleolle Sep 5, 2017

Choose a reason for hiding this comment

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

I used Kernel#warn, is that acceptable?

@olleolleolle
Copy link
Contributor Author

olleolleolle commented Sep 5, 2017

@maxmeyer Ah, reminds me: there's a --plugin junk, too. For the yardopts. Hm, perhaps that's also best left not used - we have developers on different Ruby versions.

  - Task name: lint:yard:junk
@olleolleolle olleolleolle force-pushed the fix/yard-documentation-detail branch from 7af9170 to 261cfc9 Compare September 5, 2017 20:39
@maxmeyer maxmeyer merged commit 38e86c0 into cucumber:master Sep 5, 2017
@maxmeyer
Copy link
Member

maxmeyer commented Sep 5, 2017

@olleolleolle Thanks.

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