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

fix: add JSON filename in results #3392

Merged
merged 1 commit into from
Oct 16, 2020
Merged

fix: add JSON filename in results #3392

merged 1 commit into from
Oct 16, 2020

Conversation

regseb
Copy link
Contributor

@regseb regseb commented Oct 9, 2020

Fixes #3282

The commit 087cac8 added the addonMetadata parameter to the JSONParser and LocaleMessagesJSONParser constructors. But it was not added to the JSONParser constructor call.
The addonMetadata property is no longer in use, so I removed it from both constructors and the call to the LocaleMessagesJSONParser constructor.

@wagnerand
Copy link
Member

I can't find where exactly, but after trademark checks got reverted in #3252, we decided to keep the addonMetadata around. @rpl do you recall that?

@willdurand
Copy link
Member

I can't find where exactly, but after trademark checks got reverted in #3252, we decided to keep the addonMetadata around. @rpl do you recall that?

Yes, I think we should keep it for now.

@rpl
Copy link
Member

rpl commented Oct 13, 2020

@wagnerand personally I don't recall (but I haven't been involved in neither #3252 nor #3123).

Given that this is a regression, independently from how we will agree on fixing the issue, we should also add an additional test case along with the fix (clearly it wasn't covered enough by the existing test cases).

@regseb
Copy link
Contributor Author

regseb commented Oct 13, 2020

I rollbacked the modifications and added addonMetadata to the JSONParser constructor. And I checked the filename in the unit tests.

@willdurand willdurand merged commit e154102 into mozilla:master Oct 16, 2020
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.

JSON_INVALID lint error returns null file name instead of actual JSON file name
4 participants