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

Faker adjectives #2130

Merged
merged 7 commits into from Oct 4, 2020
Merged

Faker adjectives #2130

merged 7 commits into from Oct 4, 2020

Conversation

loicboset
Copy link
Contributor

@loicboset loicboset commented Sep 29, 2020

Description:

This is a PR to add a new faker generator for adjectives.
A few weeks ago I had to create an Airbnb clone and I wanted to use Faker to generate flats description ("Nice apartment", "Cosy studio", ...) but I did not find any suitable Faker.

Hence this generator, which includes 'nice' adjectives ('clean', 'charming') and 'bad' adjectives ('horrible', 'lazy') that can be used for multiple purposes.

This is my first real PR request 🙏

I added the 4 files you will see, including the test (which passes).

However, even though I did not touch the Bank Faker, I get an error on this one after having added my changes (I ran the test before adding my changes and all tests pass, so it has to do with my changes).

/Users/loic/Desktop/Playground/faker/test/faker/default/test_faker_bank.rb:51:in `times'
     51:     10.times do
     52:       samplebic1 = @tester.swift_bic
     53:       samplebic2 = @tester.swift_bic
  => 54:       refute_equal samplebic1, samplebic2
     55:     end
     56:   end
     57: 
/Users/loic/Desktop/Playground/faker/test/faker/default/test_faker_bank.rb:54:in `block in test_swift_bic_collission'
Failure: test_swift_bic_collission(TestFakerBank):
  <"AAHVGB21"> was expected to be != to
  <"AAHVGB21">.

Obviously, I missed something so if you have any clue what to change to make all tests pass, please let me know! 💪

lib/faker/default/adjective.rb Outdated Show resolved Hide resolved
lib/faker/default/adjective.rb Outdated Show resolved Hide resolved
lib/locales/en/adjective.yml Outdated Show resolved Hide resolved
@psibi
Copy link
Member

psibi commented Oct 1, 2020

@loicboset Can you see why most of the tests are failing ?

@loicboset
Copy link
Contributor Author

@psibi yes, for a reason that I don't understand the bank faker is failing although I have not touched it.

Failure: test_swift_bic_collission(TestFakerBank):
  <"AAHVGB21"> was expected to be != to
  <"AAHVGB21">.

Do you have an idea why?

@psibi
Copy link
Member

psibi commented Oct 1, 2020

@sudeeptarlekar / @Zeragamba Do you know what's going wrong in the tests ?

@Zeragamba
Copy link
Contributor

Adjective could be a reserved class name for something?

@loicboset
Copy link
Contributor Author

I changed all the names to Adjectivees (adding one e and one s) but the same test is failing. I will try this weekend to create a different faker to check if I get the same result.

@psibi
Copy link
Member

psibi commented Oct 4, 2020

@loicboset I looked into this in more detail today and IMO the test is wrong:

  # This test makes sure there are no collissions in BIC number pool
  def test_swift_bic_collission
    10.times do
      samplebic1 = @tester.swift_bic
      samplebic2 = @tester.swift_bic
      refute_equal samplebic1, samplebic2
    end
  end

Faker doesn't guarantee that the value will be unique always: https://github.com/faker-ruby/faker#note

So, IMO that test is wrong. Free feel to comment the test out. I don't want this MR to get stagnated because of the test.

@loicboset
Copy link
Contributor Author

@psibi thanks! Everything seems good now!

@psibi
Copy link
Member

psibi commented Oct 4, 2020

@loicboset LGTM. One more change: Can you add this link: #2130 (comment) to the commented out code so that others can easily discover why it's commented out ?

@loicboset
Copy link
Contributor Author

@psibi sure! Happy to adjust anything else to comply!

@psibi psibi dismissed Zeragamba’s stale review October 4, 2020 09:07

Review has been addressed. It's ready to be merged.

@psibi psibi merged commit 671adab into faker-ruby:master Oct 4, 2020
@psibi
Copy link
Member

psibi commented Oct 4, 2020

Awesome. Thanks for your contribution.

@loicboset loicboset deleted the faker-adjectives branch October 4, 2020 09:09
@loicboset
Copy link
Contributor Author

loicboset commented Oct 4, 2020

Awesome. Thanks for your contribution.

Thanks! I am participating this month to the Hacktoberfest, are you happy to receive more PR (quality-focus)?

@psibi
Copy link
Member

psibi commented Oct 4, 2020

@loicboset Sure, that would be much appreciated!

droznyk pushed a commit to droznyk/faker that referenced this pull request Oct 23, 2020
* Adding base files for Adjective Faker

* Adding faker version

* Passing required modifications

* Commented out failing bank test

* Adding link to commented test
droznyk added a commit to droznyk/faker that referenced this pull request Oct 23, 2020
droznyk added a commit to droznyk/faker that referenced this pull request Oct 23, 2020
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.

None yet

3 participants