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

Fixing documentation - Faker::Name to Faker::Zelda #1256

Merged

Conversation

mrstebo
Copy link
Contributor

@mrstebo mrstebo commented May 26, 2018

An extension of #1252.

Here are the issues that I have found relating to the documentation.

  • Faker::Name.job_titles - not in docs
  • Faker::Nation.capital_city - duplicated in docs
  • Faker::NatoPhoneticAlphabet - incorrect title in doc
  • Faker::NatoPhoneticAlphabet.code_word - missing method name in doc
  • Faker::Number.leading_zero_number - not in docs
  • Faker::Number.decimal_part - not in docs
  • Faker::Number.non_zero_digit - not in docs
  • Faker::StarWars.call_squadron - not in docs
  • Faker::StarWars.call_number - not in docs
  • Faker::Team.sport - no code
  • Faker::University.prefix - not in docs (should be protected?)
  • Faker::University.suffix - not in docs (should be protected?)
  • Faker::University.greek_alphabet - not in docs (should be private?)

@vbrazo
Copy link
Member

vbrazo commented May 26, 2018

Amazing dude

@vbrazo
Copy link
Member

vbrazo commented May 26, 2018

These Faker::StarWars methods shouldn't be in the docs because we are using the singular methods. I don't know if we should put them in the private section. Check if we have tests for these methods. If we do, leave them how they are.

@mrstebo
Copy link
Contributor Author

mrstebo commented May 26, 2018

@vbrazo Yeah, I thought that might be the case. I had the same impression for the Number methods I mentioned. I've changed the Number methods to private and the tests still work, but never know if someone else is leveraging those methods 😅

@mrstebo
Copy link
Contributor Author

mrstebo commented May 26, 2018

@vbrazo there appear to be tests for those additional Faker::StarWars methods.

Maybe this could be another PR where we remove those methods, because they only seem to be used in conjunction with the sample method.

@vbrazo
Copy link
Member

vbrazo commented May 26, 2018

I think we shouldn't modify them for now. Let's fix the issues or add the missing tests/docs first and then we could think about improvements like this one in the future @mrstebo

@vbrazo vbrazo changed the title Fixing documentation Fixing documentation - Faker::Name to Faker::Zelda May 26, 2018
@mrstebo
Copy link
Contributor Author

mrstebo commented May 26, 2018

Also looking at Faker::Team.sport it seems that it just delegates to method_missing in the Base class because there is no code in the class for it. I'll update it so that there is an explicit method that calls fetch.

@@ -1,6 +1,6 @@
# Faker::Hobbit
# Faker::NatoPhoneticAlphabet
Copy link
Member

Choose a reason for hiding this comment

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

copy and 🍝 😆


```ruby
# A code word from the NATO phonetic alphabet
Faker::NatoPhoneticAlphabet #=> "Hotel"
Faker::NatoPhoneticAlphabet.code_word #=> "Hotel"
Copy link
Member

Choose a reason for hiding this comment

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

👍

def sport
fetch('team.sport')
end

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Thanks 🥇

@vbrazo vbrazo merged commit 05b7474 into faker-ruby:master May 26, 2018
@@ -17,4 +17,6 @@ Faker::Name.title #=> "Legacy Creative Director"

Faker::Name.initials #=> "NJM"
Faker::Name.initials(2) #=> "NM"

Faker::Name.job_titles #=> [""Supervisor", "Associate", ...]
Copy link
Member

@vbrazo vbrazo May 27, 2018

Choose a reason for hiding this comment

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

I think we should remove this method because now we have a Faker::Job API @mrstebo

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to open a PR and add a description explaining what's going on and fix it? We need to remove the locales, docs, tests and the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

When a method that has been in a gem release is being removed, we need to deprecate it before removing it so we don’t break people’s code. This would including printing a warning message in the release before it will be removed.

Copy link
Member

Choose a reason for hiding this comment

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

@mrstebo your PR would need to wait for 2 versions

@mrstebo mrstebo deleted the fixes/add-missing-documentation-batch branch May 27, 2018 07:49
@vbrazo vbrazo self-requested a review July 19, 2018 01:41
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Added missing method to name documentation.

* Removed duplicate capital_city documentation.

* Fixed title and added code_word method to Faker::NatoPhoneticAlphabet documentation.

* Added missing methods to the Faker::Number documentation.

* Added call_squadron and call_number to Faker::StarWars documentation.

* Added explicit sport method in Faker::Team. Previously it was just passing through to method_missing.

* Added prefix, suffix and greek_alphabet to Faker::University documentation.
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.

None yet

3 participants