Skip to content

Updated method template to include a warning for undocumented endpoints#185

Closed
aviflombaum wants to merge 2 commits intoslack-ruby:masterfrom
aviflombaum:undocumented-endpoint-warnings
Closed

Updated method template to include a warning for undocumented endpoints#185
aviflombaum wants to merge 2 commits intoslack-ruby:masterfrom
aviflombaum:undocumented-endpoint-warnings

Conversation

@aviflombaum
Copy link
Copy Markdown
Contributor

No description provided.

@aviflombaum
Copy link
Copy Markdown
Contributor Author

I updated the method template to include a warning to the logger for undocumented methods, wasn't quite sure how to test this as everything is autogenerated, but I was able to see the warning log happen when using this client. I'd imagine the test might be on something like the method_spec template adding a new test that logger should receive a call to warn if the method is undocumented but the tests seem to be higher level. Anyway, point me in the right direction, I'll do my best to complete this addition.

@dangerpr-bot
Copy link
Copy Markdown

dangerpr-bot commented Nov 25, 2017

1 Error
🚫 Please put back the * Your contribution here. line into CHANGELOG.md.
1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

Copy link
Copy Markdown
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Good start! First, this needs tests, please. LMK if you need help coding those.

Second if you use a method a lot you're going to flood logs, so we need a way to silence the logger for these. At the very least a global configuration to turn this off. Ideally the warning should appear once per method, too.

Comment thread CHANGELOG.md
@@ -1,6 +1,6 @@
### 0.11.1 (Next)

* Your contribution here.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Put this back please.

options = options.merge(user: users_id(options)['user']['id']) if options[:user]
<% end %>
<% if data['undocumented'] %>
logger.warn('<%= group %>.<%= name %> is an undocumented API endpoint.')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe "The <%= group %>.<%= name %> method is undocumented."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

@aviflombaum
Copy link
Copy Markdown
Contributor Author

I'm mostly confused as to where to code the tests - should I alter the method_spec.erb template to generate tests that the undocumented call warn on logger? Or is there a higher level place to code this test? I'll also make an attempt at some approach tomorrow.

I'll look until a silence option on the Slack::Logger so that it can be configured to silence warnings.

Will clean it up and squash commits too.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Nov 26, 2017

You could just add specs into https://github.com/slack-ruby/slack-ruby-client/blob/master/spec/slack/web/client_spec.rb or add an undocumented_spec.rb picking one of the undocumented methods and ensuring logger.warn is called, etc.

@dblock
Copy link
Copy Markdown
Collaborator

dblock commented Jan 22, 2018

Closing in favor of #197. @aviflombaum Take a look, is that what you had in mind? Make comments on that issue.

@dblock dblock closed this Jan 22, 2018
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.

3 participants