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

Dennis shows a warning rather than an error for a missing leading % in a placholder #101

Open
muffinresearch opened this issue Mar 24, 2017 · 4 comments

Comments

@muffinresearch
Copy link

If you have a string like (foo)s blah %(bar)s dennis returns W202 Missing variables in translated string. I would have expected this to be an error since it results in untranslated content.

This is obvious but since it's a warning it's not seen when running in errors-only mode.

@willkg
Copy link
Member

willkg commented Mar 24, 2017

Can you copy/paste the complete output? That makes it easier to build a test case and look into.

@muffinresearch
Copy link
Author

dennis version 0.9
>>> Working on: /Users/whatever/code/addons-frontend/locale/it/LC_MESSAGES/amo.po
W202: missing variables: %(timeFromNow)s
116:#: src/amo/components/AddonMoreInfo.js:62
117:msgid "%(timeFromNow)s (%(date)s)"
118:msgstr "(timeFromNow)s (%(date)s)"

W302: translated string is same as source string
552:#: src/core/containers/InfoDialog/index.js:38
553:msgid "OK!"
554:msgstr "OK!"

Totals
  Warnings:     2
  Errors:       0

Final totals
  Number of files examined:              2
  Total number of files with errors:     0
  Total number of warnings:              2
  Total number of errors:                0

Warnings  Errors  Filename
       2       0  it (amo.po)

@willkg
Copy link
Member

willkg commented Mar 24, 2017

Ahh.. ok.

Dennis was originally written to detect translated strings that when interpolated would throw an error and cause the application to return an HTTP 500. This was deemed an "error". Anything that was possibly bad, but wouldn't result in an HTTP 500 was considered a "warning". Part of the reason for that is that the warning linting rules tend to be more hand-wavey and there's more of a chance of false positives. You probably know this since you seem to be running in errors-only mode.

Anyhow, an "error" doesn't indicate that the translation is bad except the case when it's bad such that it causes an HTTP 500.

In this case, the translated string won't cause a Python exception, so it shouldn't be an error. Further, it's possible for the translated string to not want to include all the variables from the original string. I've seen that happen before.

Given that, we shouldn't change it to be an error. If it's the case that in AMO you can say with certainty that all variables in the original string should also show up in the translated string, then you could write a plugin that has this AMO-specific rule. However, Dennis doesn't quite support plugins, yet, so that'd need to be implemented first. That's covered in #9.

@willkg
Copy link
Member

willkg commented Mar 24, 2017

Relatedly, the two projects I worked on that was the impetus for writing Dennis are both dead now (SUMO and Input), so I haven't been spending much if any time on Dennis and no one else works on Dennis.

If Dennis is important to AMO (and this is true of any other project that uses Dennis), then someone should be spending some time to help out. If not, then maybe AMO should look for a different tool to use or roll their own.

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

No branches or pull requests

2 participants