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

Ignore * keys for findUnused #45

Merged

Conversation

Kerumen
Copy link
Contributor

@Kerumen Kerumen commented Aug 7, 2018

Fix a part of #44

The ternary resolution should come in another PR.

Copy link
Owner

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that, it's making the key extraction issue explicit.

@oliviertassinari oliviertassinari merged commit fb14c5f into oliviertassinari:master Aug 7, 2018
@oliviertassinari
Copy link
Owner

@Kerumen It's a great first pull request on the project 👌🏻. Thank you for giving it a shot!

@Kerumen Kerumen deleted the fix-fullyDynamicKeys branch August 7, 2018 12:02
@Kerumen
Copy link
Contributor Author

Kerumen commented Aug 9, 2018

@oliviertassinari Thanks! 🙌

Can you release as 0.6.1 please?

@oliviertassinari
Copy link
Owner

@Kerumen Sure, I'm on it!

@rpellerin
Copy link

This PR introduces a substantial regression, in that it reports many unused keys which are actually being used.

const Yolo = ({ foo }) => <div>{i18n.t(foo, { scope: 'foo.bar' })}</div>

All keys that match foo.bar.* are reported as unused.

@oliviertassinari
Copy link
Owner

oliviertassinari commented Aug 23, 2018

@rpellerin I might be wrong, but at first sight, your comment is exactly what motivated this change.
Going back to your code example. This plugin doesn't know any scope feature. Its considering it's a *. So the findUnused method is close to useless in the Doctolib codebase right now. The pull request is surfacing this core issue, exactly what we wanted.

@rpellerin
Copy link

Should I expect it to handle such cases in the near future?

@oliviertassinari
Copy link
Owner

#46

@rpellerin
Copy link

Amazing, thanks!

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.

3 participants