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

Handle hyphens in locale when fetching Articles with translated_content #610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

conarro
Copy link
Contributor

@conarro conarro commented Aug 28, 2023

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
Copy link
Contributor Author

conarro commented Aug 28, 2023

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant