mainmatter / ember-intl-analyzer

Find missing or unused translations in your Ember.js projects
MIT License
48 stars 14 forks source link

Add support for gjs/gts files #648

Closed vstefanovic97 closed 9 months ago

vstefanovic97 commented 11 months ago

How this works:

danbfield commented 10 months ago

@Mikek2252 Hey, would you be able to review this PR? It would be great to have support for the new Template Tag components.

Mikek2252 commented 10 months ago

@vstefanovic97 Looks mostly fine, but i believe globby returns a promise so the functions that return globby values should be awaited.

vstefanovic97 commented 10 months ago

@Mikek2252 they are all awaited, only thing I did was remove extraneous async on function that just return a promise but the don't have any await's inside them. The places where I removed await from function invocations are where functions don't return a promise at all

Mikek2252 commented 10 months ago

@vstefanovic97 My mistake, this looks fine to me, i'll merge this now and get a release out in the next few days

vstefanovic97 commented 10 months ago

Thank you @Mikek2252!

Turbo87 commented 10 months ago

only thing I did was remove extraneous async on function that just return a promise but the don't have any await's inside them.

this leads to errors thrown from these functions to not be converted to promise rejections though, so this needs to be done very carefully. it usually better to keep them as async functions and use return await ... inside.

vstefanovic97 commented 10 months ago

@Turbo87 I think that is probably only a problem if you are using then/catch syntax and the function ends up throwing an error and it doesn't get handled by catch.

But if you are using try/catch with async/await to handle error/promise rejection then it should be fine since still we will catch the error.

Either way I don't see a difference where this can affect the current logic, but if you prefer I can add back async keyword to the function to make sure they always return a promise?

Turbo87 commented 10 months ago

@vstefanovic97 it is a problem because it breaks the assumption about Promise-returning functions. aside from that, the async keyword is also a way to communicate to other developers that this function returns a Promise. without the keywords you have to dive into the internals of the function to figure out the return types.

tl;dr removing the async keyword can cause many subtle and often invisible issues

Turbo87 commented 10 months ago

and if you don't believe me, take it from the W3C: https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises 😉

vstefanovic97 commented 10 months ago

@Turbo87 I see your point, I added back the async keyword, do you mind running the workflow again?

danbfield commented 9 months ago

Hey, sorry to be a pain but are there any updates on this PR?

Mikek2252 commented 9 months ago

Apologies, @Turbo87 are you happy with the updates? if so i'll get this merged and create a release

vstefanovic97 commented 9 months ago

Pinging @Turbo87 again, are we ok with the changes now? cc: @Mikek2252

amk221 commented 9 months ago

Would love to get a release with this :yellow_heart:

Mikek2252 commented 9 months ago

To avoid anymore delays, im going to merge and create a release for this feel free to reach out @Turbo87 if you need me to update anything

Mikek2252 commented 9 months ago

@vstefanovic97 Thank you for the contribution, version 4.5.0 has been released now