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

Fix locale setting #2734

Merged
merged 5 commits into from
Jul 22, 2023
Merged

Conversation

mateusdeap
Copy link
Contributor

Summary

Better late than never.

This PR aims to make the locale setting work in threaded server environments.

It adds back the @locale class variable to Faker::Config and adds a new thread unsafe method that sets that variable. That way we can still set the locale on a per thread basis with Faker::Config.locale or we can set it in a global manner using Faker::Config.locale!

As a final remark, not really sure this does what we want it to do, but tests still pass.

Also would like to give a shout out to @nateberkopec, @hlascelles, @stefannibrasil and @thdaraujo for all the help and patience.

P.S.: I might need some help with writing tests. I kind of know how to write the tests, just not sure in what file to add it.

Fixes Issue #2563

@stefannibrasil
Copy link
Contributor

I've been running behind on reviews, will check out this tomorrow. In the meantime, could you take a look at the rubocop failure? Thank you 🚀

@stefannibrasil
Copy link
Contributor

stefannibrasil commented Apr 14, 2023

@mateusdeap thank you for your contribution so far! Sorry for the late response.

I think that having a configuration option which sets the default locale in a non-threadsafe way by having a:

Faker.configure do |config|
  config.locale = :en
  (...)
end

can be worth introducing on faker. It seems like that's the easiest way to set that default locale initially. @thdaraujo thoughts? It would be a breaking change, and we can create a new release for it, no worries.

Something I had in mind is to include this test from this branch, as it shows the desired behaviour of setting the locale globally with per-thread locales. It's just a start but it gives us a start 💭

If I add this test when trying the branch locally, I have some failing specs, though. Updated: see comment below.

About your question of where to place the tests, there is the faker test file. Another option is add the tests to the test_locale file. But I am okay with creating a new test file just for testing threads. What do you think?

@stefannibrasil
Copy link
Contributor

Sorry, I first missed the ! in Faker::Config.locale!.

These are now passing:

  def test_setting_default_locale
    # if locale is not set, fallback is :en
    assert_equal :en, Faker::Config.locale

    # locale can be updated initially
    # and it becomes the default value
    # for new threads
    Faker::Config.locale!(:pt)

    assert_equal :pt, Faker::Config.locale

    t1 = Thread.new do
      # child thread has initial locale equal to
      # latest locale set on main thread
      # instead of the fallback value
      assert_equal :pt, Faker::Config.locale

      # child thread can set its own locale
      Faker::Config.locale = :es
      assert_equal :es, Faker::Config.locale
    end

    t1.join

    # child thread won't change locale of other threads
    assert_equal :pt, Faker::Config.locale

    t2 = Thread.new do
      # initial default locale is copied over to new thread
      assert_equal :pt, Faker::Config.locale

      Faker::Config.locale = :it
      assert_equal :it, Faker::Config.locale
    end

    t2.join

    assert_equal :pt, Faker::Config.locale

    # setting this to reset the default locale for all tests
    Faker::Config.locale!(nil)
    assert_equal :en, Faker::Config.locale
  end

  def test_setting_default_locale_on_child_thread
    # if locale is not set, fallback is :en
    assert_equal :en, Faker::Config.locale

    # locale can be updated initially
    # and it becomes the default value
    # for new threads
    Faker::Config.locale!(:pt)

    assert_equal :pt, Faker::Config.locale

    t1 = Thread.new do
      # child thread has initial locale equal to
      # latest locale set on main thread
      # instead of the fallback value
      assert_equal :pt, Faker::Config.locale

      # child thread can set the default locale
      Faker::Config.locale!(:es)
      assert_equal :es, Faker::Config.locale
    end

    t1.join

    # all threads now will have the same default
    assert_equal :es, Faker::Config.locale

    t2 = Thread.new do
      # initial default locale is copied over to new thread
      assert_equal :es, Faker::Config.locale

      Faker::Config.locale = :it
      assert_equal :it, Faker::Config.locale
    end

    t2.join

    assert_equal :es, Faker::Config.locale

    # setting this to reset the default locale for all tests
    Faker::Config.locale!(nil)
    assert_equal :en, Faker::Config.locale
  end

We'll need to update the docs as well to guide folks on how to use this as well. I confess I'm not super familiar with this, so hopefully @thdaraujo can chime in to give some ideas on the tests and docs 🙏

Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

thanks for working on this and being so helpful, @mateusdeap!

For testing, we can just add a new file called faker/test/test_default_locale.rb and base it off of the specs that @stefannibrasil shared. That would be good enough.

See my comment about changing to default_locale and let me know what you think.

lib/faker.rb Outdated Show resolved Hide resolved
lib/faker.rb Outdated Show resolved Hide resolved
lib/faker.rb Outdated Show resolved Hide resolved
lib/faker.rb Outdated Show resolved Hide resolved
@stefannibrasil
Copy link
Contributor

Merging this and I will add the test and update the README in a follow up PR. Thank you @mateusdeap for your contribution!

@stefannibrasil stefannibrasil merged commit 76a168b into faker-ruby:main Jul 22, 2023
@stefannibrasil stefannibrasil linked an issue Jul 22, 2023 that may be closed by this pull request
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.

Locale setting can be ignored
3 participants