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

const-rgx does not work with the constant names in keydata.py. #125

Closed
wants to merge 2 commits into from

Conversation

the0id
Copy link

@the0id the0id commented Feb 15, 2017

Adding a '_' to const-rgx fixes this.

@the0id
Copy link
Author

the0id commented Feb 15, 2017

It looks like I'm having the same problem that rodrigc had in #123 with missing .result files.

@rodrigc
Copy link
Contributor

rodrigc commented Mar 4, 2017

@the0id can you rebase this change to the master branch? I fixed a bunch of stuff.

@the0id
Copy link
Author

the0id commented Apr 7, 2017

Ok I updated the base and ran again and now there's a new error.

[FAIL]
Traceback (most recent call last):
  File "/home/travis/build/twisted/twistedchecker/.tox/py35-tests/lib/python3.5/site-packages/twistedchecker/test/test_functionaltests.py", line 57, in wrapper
    wrapped(*args, **kwargs)
  File "/home/travis/build/twisted/twistedchecker/.tox/py35-tests/lib/python3.5/site-packages/twistedchecker/test/test_functionaltests.py", line 183, in _runTest
    testCase.fail(_formatResults(moduleName, expectedResult, outputResult))
twisted.trial.unittest.FailTest: 
twistedchecker.functionaltests.constname
= Expected =        = Actual =
5:C0103             
7:C0103             
9:C0103       

I don't know what this means. It seems to have something to do with constnames, which is related to what I changed but I don't know if it's my change doing this.

@adiroiban
Copy link
Member

@the0id there is a set of functional/end to end tests in which twistedchecker is executed over couple of predefined files.

then it compares the values generated from those files, to a list of expected errors.

So in the testing folder you will see each NAME.py file together with a NAME.result file. those are the expected results.

This are the tests... a bit strange, but this is what we have.

In the test failure report you should also see the name of the file which failed.

Tests are here https://github.com/twisted/twistedchecker/tree/master/twistedchecker/functionaltests

You can manual run twistdchecker agains those files and see if the result is valid... maybe the tests were broken

@the0id
Copy link
Author

the0id commented Apr 10, 2017

@adiroiban thanks, I found the file and the first bad example in there is "foo_bar" (no quotes).

This seems to suggest that the check is behaving as intended because the constant names I'm trying to add follow that format. But just about every constant name in keydata.py follows that format. I'm assuming they're not causing errors because they were committed before the checks were in place.

Now the question seems to be which is right. The constant names in keydata.py or "foo_bar" not being allowed?

@adiroiban
Copy link
Member

In the beginning you would have SOMETHING_fail.py and SOMETHING_pass.py ... but while I have worked on this code I tried to merge them and have only SOMETHING.py and then a comment before each example.

foo_bar is not valid, as it should be camel case or all caps


I know that the names from keydata.py are not valid... but that is testing code, so we can rename them.

There is also this ticket in which the direction is to relax the rule for test code


But the main question is. Do we still want twistedchecker. Does it really help?
These are some questions for the mailing lists.

@the0id
Copy link
Author

the0id commented Apr 11, 2017

I don't want to get involved in that discussion, but I will say that I think txchecker is useful, but it seems that it's being developed in production. I would put it on hold for now, decide what you want it to do and then bring it back.

I'll let you figure out what to do, and then do what you tell me. :)

@adiroiban
Copy link
Member

I think that for this case, the decision/discussion already took place.

That is, for the main code, reject _ (underscores) ... but allow them in the testing part.

Just changing the pylint config will not work as it will be applied across the whole project... instead of just to the testing code.

I think that we should get rid of const-rgx (allow anything) and implement a custom checker similar to what we have here https://github.com/twisted/twistedchecker/blob/master/twistedchecker/checkers/names.py

@the0id
Copy link
Author

the0id commented Apr 12, 2017

That's fine by me. If you want to go that way then I'll close this PR and branch.

Since this doesn't really affect me, nor do I have the free time to focus on it, I'll leave it for someone else to fix. :)

@the0id the0id closed this Apr 14, 2017
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

3 participants