Skip to content

Handle hyphens in locale when fetching Articles with translated_content - #610

Open
conarro wants to merge 1 commit into
intercom:masterfrom
conarro:master
Open

Handle hyphens in locale when fetching Articles with translated_content#610
conarro wants to merge 1 commit into
intercom:masterfrom
conarro:master

Conversation

@conarro

@conarro conarro commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

Certain locales include hyphens, which causes trouble (i.e. a SyntaxError) when dynamically defining method names.

This change converts the hyphen to an underscore for safety.

Why?

Without this, calling e.g. intercom_client.articles.all.to_a will raise a SyntaxError if the articles have translated content, as it tries to define a method with a hyphen for certain locales (e.g. pt-BR).

How?

The update checks for - in attributes and, if present, replaces with _ before defining accessors and using the setter.

I experimented with pushing this lower (into the DynamicAccessors) but the ApiResource#initialize_property immediately calls set_property using the same attribute name, so we'd then end up having to do this sanitization in multiple places which felt weird.

Other thoughts

When writing the specs I was tempted to add a new directory, e.g. spec/fixtures/articles/, and store the sample article JSON there vs. pasting it directly in spec_helper. For simplicity/consistency I mimicked the current approach but wanted to share my instinct in case useful input 😅.

I also considered trimming the test article hash down to just a couple representative locales to keep it shorter (i.e. a local without a hyphen and a locale with a hyphen), but I thought it better to keep the representation as a mirror of what is actually returned.

Certain locales include hyphens, which causes trouble (i.e. a `SyntaxError`) when dynamically defining method names.

This converts the hyphen to an underscore for safety.
@conarro

conarro commented Aug 28, 2023

Copy link
Copy Markdown
Contributor Author

Regarding the spec failure:

/usr/local/bundle/gems/mocha-1.16.1/lib/mocha/integration/mini_test/adapter.rb:26:in `included': uninitialized constant MiniTest (NameError)

It looks like mocha < 2.1.x is not compatible with minitest >= 5.19.x: freerange/mocha#614

This is fixed in mocha 2.1.0.

So there needs to be a change to the gemspec to constrain one (or both) of these dependencies to ensure compatibility. If the decision is to pin minitest < 5.19, note that you'll encounter another compatibility problem, this time with the m gem, where m < 1.6.1 is incompatible with minitest >= 5.16.x: qrush/m#93

This is fixed in m@1.6.1 as mentioned here.

For example, here's what I did locally to get specs working:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant