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

Allow --varformat='' to disable all formats #84

Closed
wants to merge 1 commit into from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Oct 13, 2016

Also permit empty formats, such as --varformat=',',
to be discarded instead of raising an Exception.

Fixes #83

Also permit empty formats, such as --varformat=',',
to be discarded instead of raising an Exception.

Fixes mozilla#83
@willkg
Copy link
Member

willkg commented Oct 22, 2016

This needs tests. I'll look at writing those now.

@willkg
Copy link
Member

willkg commented Oct 23, 2016

I hit some issues while writing up a test. There's a lot of code that expects at least one varformat. I think this is going to need some rethinking.

The test I wrote is this. It goes at the bottom of test_cmdline.py:

    def test_varformat(self, runner, tmpdir):
        po_file = build_po_string(
            '#: foo/foo.py:5\n'
            'msgid "Foo %(o)s baz"\n'
            'msgstr ""\n')
        fn = tmpdir.join('messages.pot')
        fn.write(po_file)

        result = runner.invoke(cli, ('lint', '--varformat=', str(fn)))

        # FIXME: Print exception the code threw.
        import traceback
        traceback.print_exception(*result.exc_info)

        # FIXME: Test fails at next assertion because exit_code is -1 because
        # the code threw an exception.
        assert result.exit_code == 0
        # We're not looking at any variables, so we should never have a
        # variable related warning.
        assert 'W501: one character variable name "o"' not in result.output

I'm pretty sure that test is correctly written.

That's throwing this error:

Traceback (most recent call last):
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/testing.py", line 279, in invoke
    prog_name=self.get_default_prog_name(cli), **extra)
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/willkg/.virtualenvs/dennis/local/lib/python2.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/willkg/projects/dennis/dennis/cmdline.py", line 259, in lint
    results = templatelinter.verify_file(fn)
  File "/home/willkg/projects/dennis/dennis/templatelinter.py", line 175, in verify_file
    msgs.extend(self.lint_poentry(entry))
  File "/home/willkg/projects/dennis/dennis/templatelinter.py", line 157, in lint_poentry
    msgs.extend(lint_rule.lint(self.vartok, linted_entry))
  File "/home/willkg/projects/dennis/dennis/templatelinter.py", line 83, in lint
    if len(token) == 1 and token.isalpha():
TypeError: object of type 'NoneType' has no len()

I'm pretty sure this is because the variable tokenizer creates a regexp even if there are no variables to parse out.

I have a few thoughts on this. First, this is hitting up against poor architecture on my part. For a while now, we've needed to convert from my faux parser to a real parser. I started working on that, but I don't think I got very far. The gist of it is that there are a lot of different things that can participate in tokenizing a string, so we need to build a parser that can have arbitrary sets of rules depending on supported variable formats and whatever else the lint rule is doing. It's tricky and I haven't done real parser work in almost a decade.

Can you work on this further? If not, I might find some more time this weekend to look into it, but if I can't, then it might be a while.

@willkg
Copy link
Member

willkg commented Nov 6, 2016

I spent some time on this. I thought this was hitting bad architecture, but it's actually not. It's just hitting problems with the variable tokenizer because I didn't expect the "no formats" case.

I fixed it in #92. Closing this out.

@willkg willkg closed this Nov 6, 2016
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.

None yet

2 participants