Add support for concat expressions in HBS files

[robinborst95]

After adding support for (in-repo) addons, I noticed that dynamically constructed translations were not recognized. In HBS files this is usually done with the concat helper, which is often just a combination of strings literals and if expressions (e.g. {{t (concat "actions." (if @isEditing "save" "publish"))}}), and this PR adds support for that.

Note that like my 2 other PRs, I haven't added tests or (self-review) comments yet, I'll do that when this gets a chance of being actually merged. Edit: I added tests that contain some examples of what this PR actually adds support for.

[bertdeblock]

I've actually always liked the fact that ember-intl-analyzer doesn't recognize or tries to recognize logic like this.
I don't feel that {{t (concat "actions." (if @isEditing "save" "publish"))}} is a good way to dynamically render a translation. I feel that (concat ...) here makes the code overly complex and harder to read, only to avoid writing actions. twice.

{{t (if @isEditing "actions.save" "actions.publish")}}

or

{{if @isEditing (t "actions.save") (t "actions.publish")}}

Seem like much better ways of writing this to me.

But, I understand that this tool is not a linter, so these changes probably make sense?
Just wanted to share some thoughts on this 😄.

[robinborst95]

Good point, I actually agree that my example is easier to read without the concat 👍 In fact, I was changing such occurrences in our codebase to not use concat so that I didn't have to whitelist them.

However, we also have places where the translation keys are longer and where it makes more sense to not duplicate the entire key, and I can imagine that more codebases have similar use cases. As I was already a bit familiar with the code after my other 2 PRs, I considered adding support for it here, so maybe it can still be a nice addition 🙂

[Mikek2252]

Hmm this one i'm a little conflicted on, while i think it is helpful to have this as im sure they are plenty of people that do it and it will be helpful for them as they wouldn't need to whitelist anything. But we are not able to support dynamic concatenation e.g. "string" + variable and i feel like having this change may make people attempt it and have the expectation that it should work. I'll have a little think about this one and get back to you early next week.

One small thing about the actual changes, for completeness would you also have this for js concatenation? I'd assume this doesnt work for js as it stands

[robinborst95]

Yeah, the dynamic concatenation is indeed something that this doesn't support. However, I did make sure that it is recognized, in which case the entire expression is ignored, so this might open up opportunities to try and notify/hint about their existence.

About the JS concatenation, I didn't run into these cases in our codebase, so I haven't looked at it. I'm not sure how often this occurs though, and if it occurs, I don't know if only the ternary condition ? 'new' : 'edit' would be something to support, there are probably other ways too. In the HBS there are less options it seems, from my experience at least, so that's why that was easier to implement.

When you have time to have a look, then looking at the test cases might be a good start, as that clearly indicates what it actually supports then.

[robinborst95]

@Mikek2252 in #514 I've added support to detect dynamic translations, which showcases the usefulness of this PR. With that in mind, would you have time to look at this PR 🙏 ?

[Mikek2252]

Hey sorry, i had a thought about this, would it be okay to make this "opt in" using the config file, that way it would discourage people from using unless they have a specific need for it

[robinborst95]

That sounds reasonable 👍 In order to detect dynamic translations properly it's best to have this enabled though, but I've built that opt-in too, so I can also include the concat expressions when dynamic translations are logged.

About the name, something like analyzeConcatExpression: true in the config?

[robinborst95]

@Mikek2252 I've added the configuration option to make it opt-in, including a section on it in the Readme, curious what you think 👍 .

[robinborst95]

@Mikek2252 any chance that you can have a look at this?

[Mikek2252]

@robinborst95 Hi thanks for this i will merge this and create a release, sorry i have been away a lot the last few weeks and havent had much time for open source