Improved error message for invalid comments #47

tobiasherp · 2018-05-29T11:12:06Z

When invalid comments are found in the assigment to the zcml variable
(i.e. those prepended by whitespace in the same line), the recipe
complains about "Invalid zcml" and a "package" which is named
'#firstword' or (even worse, if the '#' is followed by whitespace) '#',
and the rest of the comment is silently taken as package names as well.
(issue 46)

This change explicitly creates a hopefully much more useful error
message.

When invalid comments are found in the assigment to the zcml variable (i.e. those prepended by whitespace in the same line), the recipe complains about "Invalid zcml" and a "package" which is named '#firstword' or (even worse, if the '#' is followed by whitespace) '#', and the rest of the comment is silently taken as package names as well. (issue 46) This change explicitly creates a hopefully much more useful error message.

mauritsvanrees · 2018-06-01T00:34:28Z

Wouldn't it be fine to ignore the comments instead of raising an error? So this:

if package.startswith('#'):
    continue

Perhaps print a warning, but I don't think even that is needed.

tobiasherp · 2018-06-04T11:27:15Z

@mauritsvanrees :
No, I'm afraid a warning is not good enough. The rest of the (invalid) comment line will be part of the "package" list as well, and thus e.g. commented-out packages can get included.

We can't rely on anything sane to happen when a comment is prepended by whitespace; so we should abort the build with a good message.

mauritsvanrees · 2018-06-04T13:14:51Z

I expect that package contains something like # my.package. It would be fine to ignore that, right?
Well, it will likely be # my.package, so we should strip white space before checking for a #.

Or is zcml split on white space? Then you would be right.

tobiasherp · 2018-06-04T14:38:58Z

@mauritsvanrees:

Of course comments like # my.package are perfectly common; but they only work right when the # is the 1st character in the line. If prepended by whitespace, they yield two "packages": # and the should-be-commented-out my.package.
If we'd simply ignore the #, the commenting-out would silently fail to work.
PEP 20.10, "Errors should never pass silently."
Yes, the zcml value is apparently split on white space.

mauritsvanrees · 2018-06-07T21:39:13Z

Yes, the zcml value is apparently split on white space.

Okay, I didn't know that. In that case, as I said, you are right. So raising an error is good and helpful here.

I am not sure about the wording of the new error message though. I have this test zcml:

zcml =
# Hoppa
    foo.bar
    # wrong.comment
    fine.here

The error then becomes:

ValueError: Invalid comment in zcml assignment (must start in first column; something like '# wrong.comment fine.here')

I'm not sure everyone will realise what you mean with the first column. Instead of "must start in first column" maybe: "comment sign must be the first character on the line"? To me that would be clearer. I don't feel strongly about this, just a suggestion.

And I think only the first package after the comment-sign is relevant. At least I assume most users will not have zcml = a b # c d with an inline comment, but only make the error with a multiline zcml statement. My assumption may be wrong though.

I wondered if the same needs to be done for the eggs option, but I see that one goes wrong in zc.recipe.egg (with ValueError: need more than 0 values to unpack), so that one is out of control of this package. So never mind that.

mauritsvanrees · 2018-06-07T21:39:23Z

Please start the tests on Jenkins:

Log in to jenkins.plone.org with your github account.
Go to the 'Build with parameters' form for all relevant Plone versions. You should see direct links to those versions under the 'Some checks haven’t completed yet' box of the pull request.
Paste the pull request url in that form and build it.

mauritsvanrees · 2018-06-07T21:44:18Z

src/plone/recipe/zope2instance/__init__.py

+                    raise ValueError('Invalid comment in zcml assignment'
+                                     ' (must start in first column; something like'
+                                     ' %(comment_snippet)r)'
+                                     % locals())


Nit picking: why use locals() here? Since the string only uses one variable, I would just say ...%r' % comment_snippet or '{0!r}'.format(comment_snippet).

Call it lazyness; I'll do it like this most of the time. When logging, it's a comma instead of the % sign. Also, it's more foolproof to use a dict than a simple variable (which could contain a tuple and make the whole thing fail). Last, it should still be pretty efficient.

Concerning jenkins, I'll need some more guidance here. I clicked over to jenkins.plone.org, and I was logged in without any futher action with my Github acount, but I can't find that "Build with parameters" form. Is this the exact name?

On https://jenkins.plone.org/job/plone-4.3-python-2.7/lastCompletedBuild/testReport/, there is no link for plone.recipe.zope2instance.

I had a look at https://docs.plone.org/develop/coredev/docs/intro.html#jenkins-mr-roboto as well but couldn't find a hint there.

For Jenkins, see http://jenkinsploneorg.readthedocs.io/en/latest/run-pull-request-jobs.html. The video is helpful.

Or look at the box 'Some checks haven’t completed yet' on the github page you are currently reading. It has direct links to the relevant Plone versions that need a Jenkins run.

... which is presented in the ValueError, to one whitespace-separated "word" only ('#word' or '# word'). Reordered the error message.

Currently, the value is simply split by whitespace; thus, the "package" cannot contain blanks, and we can't tell how many of the following words are part of the comment. If the zcml value one day would be split by lines only, we would be able to safely skip the end of the line, following the '#'. If the comment doesn't contain blanks, we can't know this; but the only way for a comment to contain blanks is splitting by lines. So we can safely accept the '#text with blanks' or '# text with blanks' "package" as a comment and skip it.

Instead of performing complicated checks of package names starting with '#' characters, we can quite as well make line comments work. The function nocomments_split first splits the value by line separators, and then removes everything after the '#' (see doctest).

With the nocomments_split function, the mentioned error message is not necessary anymore, since comments are weeded out above.

tobiasherp · 2018-07-24T13:37:01Z

I found a better solution: instead of forging a hopefully helpful message, simply make line comments in the zcml value work, whether or not they start at the beginning of the line.

The function nocomments_split is very simple, documented by doctests, and could be used for other values as well.

tobiasherp · 2018-07-27T15:59:44Z

@mauritsvanrees:
I hope you like my current solution; sorry about the commit noise (everything interesting is in the last two).
That nocomments_split function could perhaps be used for other vars as well, but of course I only used it for zcml for now.

mauritsvanrees

I like it!
And it seems to work fine.
This solution would be useful for the eggs option too, but that can be done in a later PR if you are up to it. I will gladly merge this one now. Thanks!

Can you make a PR for master too for the zcml?

tobiasherp · 2018-11-16T14:40:17Z

@mauritsvanrees: Sorry for the delay. I just created the pull request #57

tobiasherp added 2 commits May 29, 2018 13:01

Link to issue 46

59a5fed

jensens added 32 needs: review 24 status: ready 04 type: enhancement labels Jun 7, 2018

mauritsvanrees reviewed Jun 7, 2018

View reviewed changes

tobiasherp added 4 commits July 24, 2018 13:37

Shortened the comment_snippet

6028a5b

... which is presented in the ValueError, to one whitespace-separated "word" only ('#word' or '# word'). Reordered the error message.

Changed the description

3c934dc

With the nocomments_split function, the mentioned error message is not necessary anymore, since comments are weeded out above.

mauritsvanrees approved these changes Aug 3, 2018

View reviewed changes

mauritsvanrees merged commit e96bdac into plone:4.x Aug 3, 2018

gotcha mentioned this pull request Sep 10, 2018

the fix for bad comments in zcml key should also be applied to eggs key #50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved error message for invalid comments #47

Improved error message for invalid comments #47

tobiasherp commented May 29, 2018

mauritsvanrees commented Jun 1, 2018

tobiasherp commented Jun 4, 2018 •

edited

Loading

mauritsvanrees commented Jun 4, 2018

tobiasherp commented Jun 4, 2018 •

edited

Loading

mauritsvanrees commented Jun 7, 2018

mauritsvanrees commented Jun 7, 2018

mauritsvanrees Jun 7, 2018

tobiasherp Jul 11, 2018 •

edited

Loading

tobiasherp Jul 11, 2018 •

edited

Loading

mauritsvanrees Jul 16, 2018

tobiasherp commented Jul 24, 2018

tobiasherp commented Jul 27, 2018

mauritsvanrees left a comment

tobiasherp commented Nov 16, 2018

Improved error message for invalid comments #47

Improved error message for invalid comments #47

Conversation

tobiasherp commented May 29, 2018

mauritsvanrees commented Jun 1, 2018

tobiasherp commented Jun 4, 2018 • edited Loading

mauritsvanrees commented Jun 4, 2018

tobiasherp commented Jun 4, 2018 • edited Loading

mauritsvanrees commented Jun 7, 2018

mauritsvanrees commented Jun 7, 2018

mauritsvanrees Jun 7, 2018

Choose a reason for hiding this comment

tobiasherp Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

tobiasherp Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

mauritsvanrees Jul 16, 2018

Choose a reason for hiding this comment

tobiasherp commented Jul 24, 2018

tobiasherp commented Jul 27, 2018

mauritsvanrees left a comment

Choose a reason for hiding this comment

tobiasherp commented Nov 16, 2018

tobiasherp commented Jun 4, 2018 •

edited

Loading

tobiasherp commented Jun 4, 2018 •

edited

Loading

tobiasherp Jul 11, 2018 •

edited

Loading

tobiasherp Jul 11, 2018 •

edited

Loading