Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • 04:26
    shioyama synchronize #548
  • 04:26

    shioyama on fix_fallbacks_1_2

    Add 1-1, 1-2 branches (compare)

  • 04:24
    shioyama opened #548
  • 04:24
    shioyama closed #547
  • 04:24
    shioyama opened #547
  • 04:23

    shioyama on fix_fallbacks_1_2

    Fix fallbacks performance regreā€¦ (compare)

  • 04:23
    shioyama commented #546
  • 04:22
    shioyama commented #544
  • 04:13
    shioyama commented #544
  • 04:00
    shioyama commented #544
  • 04:00
    shioyama commented #544
  • Nov 23 13:25
    f1sherman commented #544
  • Nov 23 05:51
    shioyama commented #544
  • Nov 23 05:41
    shioyama commented #544
  • Nov 21 22:23
    f1sherman commented #544
  • Nov 21 22:17
    shioyama commented #544
  • Nov 21 14:48
    f1sherman commented #544
  • Nov 21 12:04
    shioyama commented #544
  • Nov 21 11:47
    shioyama commented #544
  • Nov 17 18:04
    f1sherman commented #546
Chris Salzberg
@shioyama

Hey, sounds like a bit of a challenge you've got there. Let me respond point by point.

  • Fallbacks as a proc is something I'd like to do and there's a PR for it, but the implementation needs some work: shioyama/mobility#328
  • About the empty product names thing, this is a common pain point and something I'd like to eventually provide support for (i.e. migrating from a "normal" column attribute to a translated one). I don't have a solution for it but I consider it a problem that Mobility should solve or at least support.
  • Json with MySQL is not supported currently. There was a PR for it but it never really got anywhere: shioyama/mobility#271 This is not a priority for me, if you want it I'd ask you to make a PR which I can help with.
  • Rails 4.2 support is and will continue to be incomplete, and at some point I will start dropping the conditionals and remove tests for it, so don't really count on it. Sorry!

Those are my quick thoughts. I won't be doing anything Mobility-related for a while probably, and first priority is cleaning up the code to release v1.0, so the first and second points above would come after that (or possibly as a part of that).

Mark Pitsilos
@yourtallness
Thanks for the quick response @shioyama
  • Is the PR for fallbacks per-attribute or will we also be able to set a proc on a global level?
  • I suppose the native column is not even required to be present with globalize or mobility, right? It is in essence a virtual attribute
  • I read in the readme that a proc can be provided as a fallback generator, but don't quite understand how it's supposed to work
    Sorry for the barrage of questions!
    Seems we may need to roll our own solution with MySQL json using the native column primarily and getting / setting the translations as needed
Chris Salzberg
@shioyama
  • the fallbacks proc thing works exactly like other fallbacks, per attribute, can be set globally as well
  • native column is not required at all, and not assumed to be present (for Mobility). I'm not following Globalize anymore so can't say for that gem.
  • the proc as fallbacks generator was a bad design decision, and it's not what you're looking for. It's something else and mostly just confusing so probably will be removed.
Tim Krins
@timkrins
Hi there - I was wondering if there was currently a way to globally override fallbacks for a block?
I have done this in my own fork - I can use Mobility::Plugins::Fallbacks.with_fallbacks(:en) { # fallbacks are overridden }
but if it is possible another way that would be interesting. If not, I can create a feature PR.
Also, I notice that master uses a different plugin config (no longer an assignment, but is a function) but this is not referred to in docs (the docs seem to lag master which makes sense for rubygems users)
Chris Salzberg
@shioyama

To answer your second question (quickly): master is a WIP toward a 1.0 release, so there are some fundamental things that have changed there. They are not yet documented. All released versions are on the 0.8 branch, and I'm keeping 0-8-stable up to date with any new changes I merge in.

It's complicated and wish I could get out the 1.0 release, but just no time right now so for now 0.8 is stable and the one you should use. Make PRs to master and I'll port back to 0.8 and release there for now.

Bram Jetten
@Bramjetten
Hey @shioyama ! I've just updated one of my projects to 0.8.12, but that broke something seemingly unrelated
This change triggers the error:
I'm getting a "wrong number of arguments (given 2, expected 1)" for an attribute that's set using SQL
Some info: I have a huge SQL query to calculate a stock forecast for a list of products (which have translated names)
Schermafbeelding 2020-05-18 om 10.59.34.png
Every time I try to use attributes defined here (like 'past_30_days'), I get the error above
I've reverted to 0.8.11 now. Should I create an issue on GH?
Tim Krins
@timkrins
@Bramjetten whats your ruby version?
Tim Krins
@timkrins
and also, by 'use an attribute like past_30_days', are you calling the accessor with any arguments?
Bram Jetten
@Bramjetten
@timkrins 2.6.5
No arguments
I'm seeing these errors in a lot of places in my apps
I reverted all of our apps to 0.8.11
Tim Krins
@timkrins
hmm weird. sorry I can't help more. that change in itself I really would not expect anything to happen.
you could try and find out what the second argument that is being passed in actually is.
because that error seems to me to indicate that options in the fallthrough accessor actually has some value (otherwise it would be empty and effectively would not be passed to the super)
Bram Jetten
@Bramjetten
Yes, I'll try to figure it out
Bram Jetten
@Bramjetten
I modified _read_attribute to show me what's being passed:
Schermafbeelding 2020-05-20 om 13.49.03.png
Schermafbeelding 2020-05-20 om 13.49.10.png
Happens everywhere I set attributes using AR select
Another example:
Bram Jetten
@Bramjetten
@products.select("COUNT(*) AS count_all, spina_shop_products.properties->'manufacturer' AS filter").group("filter")
This query uses jsonb, but seems unrelated to the issue
Also, these properties (manufacturer/filter) have nothing to do with mobility, other than that products have a couple of translated attributes
Chris Salzberg
@shioyama
Hmm just catching up
So this is the change in 0.8.12: shioyama/mobility#364
Chris Salzberg
@shioyama

Ok, so I have some insight into this problem actually. I noticed you're aliasing query results in that SQL (as coming_30_days etc). That means those attributes will be actually fetched using method_missing rather than defined methods, which explains the relation to the change in 0.8.12.

For background on that (purely discussion of ActiveRecord), watch my RailsConf talk on the topic: https://www.youtube.com/watch?v=PNNrmNTQx2s&feature=emb_logo

So if you watch that talk, you'll understand (hopefully!) why calling coming_30_days on your model will end up hitting method_missing. For a model with Mobility enabled, and using fallthrough accessors (which are there if you're using dirty tracking), you will pass through that code that was changes. So that's the connection.
Chris Salzberg
@shioyama
Next question is: why did that code change anything?
Chris Salzberg
@shioyama
I'm wondering if part of the problem here is that the method signature for method_missing is not supposed to actually include keyword arguments:
method_missing(m, *a, &b)
So maybe what mobility is doing here, catching a method call with keyword arguments using method_missing, is actually breaking the API. Previously it was hiding that by not passing the keyword arguments to super, but now since it is it can break other stuff. Ironically that was what the original PR was trying to fix.
Chris Salzberg
@shioyama
Ok here's a failing test for the issue (on master branch, but same problem): shioyama/mobility@6730424
Basically it's a keyword arugment / hash thing. When you define method_missing without any **options, the options are being converted to a hash, which does not match the signature that ActiveRecord expects.
Chris Salzberg
@shioyama
Ok here's a tentative fix: shioyama/mobility#374
Chris Salzberg
@shioyama
Basically, we assume that if the method matches a Mobility locale accessor call, the last argument is an options hash (keyword arguments or hash object). This is safer since we can confine our thinking to what Mobility does. Then if it doesn't match, we just pass all the arguments along. This should satisfy both expectations.
Chris Salzberg
@shioyama
Ok Travis seems to be dead :disappointed: but tests pass locally for me. @Bramjetten can you try that PR branch in one of your repos? I think it should work for both scenarios.
Bram Jetten
@Bramjetten
I'm far from as well versed in ActiveModel/Record as you are, but I think I follow! Sadly, your fix introduced two new bugs for me.
Using a default for translated attributes stopped working and somehow the query_method is unavailable when saving a translated model.
I posted screenshots on your PR
Chris Salzberg
@shioyama
Ah sorry, the fix should be fine but that PR was off of the master branch, which has many other (unreleased) changes. I'll make a separate PR for 0.8.x and you can try that one.
Chris Salzberg
@shioyama
Unfortunately I won't be able to finish this until Monday, so I'm going to yank 0.8.12 for now.