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

Fallback to callee definition when definition not found (fixes #131) #149

Merged
merged 18 commits into from
Mar 11, 2013

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Mar 10, 2013

This is my try to fix #131.

  • There are some known failures due to some bugs in parser
  • To skip known failures, unittest2 is used for Python 2.5 and 2.6


def test_definition_when_in_function_call_right_after_comma(self):
defs = self.definition_when_in_function_call('f(1,')
self.assertEqual(defs[0].description, 'def f')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use the blackbox tests (run.py) for that? It's documented in the dev documentation. I just don't like these unit tests at all. Totally confusing.

You could just add a new blackbox file for your tests if you want or just put it into an existing file.
If you want to skip tests, just use a double comment: ##! str

It also reminds me that I have to implement the tests for call_signatures (function_definition).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried. But #! f did not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed I can use builtin functions so I added some blackbox tests. However, the blackbox tests are not enough since you can't specify column (or can you?).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the blackbox tests are not enough since you can't specify column (or can you?).

You can, but maybe it's not documented good enough:

#? 5 str()
str( )

@tkf
Copy link
Contributor Author

tkf commented Mar 10, 2013

OK, I added some more blackbox tests. But there is still some missing tests for unclosed paren, since you can test only one case per file. If you think it is OK to remove these cases, I will just remove the tests I added from regression.py.

But I really think jedi use parametrised test in addition to blackbox tests. For example, if you use nose's generator test, you can write it like this. I found it far more easy to write and read than the blackbox tests I wrote.:

    def check_definition_when_in_function(self, *args):
        defs = self.definition_when_in_function_call(*args)
        self.assertEqual(defs[0].description, 'def f')

    def test_definition_when_in_function(self):
        for param in [
                ('f(',),
                ('f( ',),
                ('f(1,',),
                ('f(1, ',),
                ('f(', ')'),
                ('f( ', ')'),
                ('f(', ' )'),
            ]:
            yield (self.check_definition_when_in_function,) + param

I don't think this holds always:

it is just stupid to write 200‘000 unittests in the manner of regression.py

@davidhalter
Copy link
Owner

I found it far more easy to write and read than the blackbox tests I wrote

Well, I personally think that the case above

But there is still some missing tests for unclosed paren, since you can test only one case per file

That's not true. You just have to know how to handle it. just use def x(): pass in between the tests. I know this sounds stupid, but def is a "stop-word". However, this might not be the best solution.

But there is still some missing tests for unclosed paren, since you can test only one case per file:

Yeah you're probably right for these cases. But your tests didn't look like that. You used 8 different tests. That was my problem. Just the same code over and over again. I don't dislike your example above.

However, I personally like to have as much as possible in one place, but the above example is (because it's really short) ok.

@tkf
Copy link
Contributor Author

tkf commented Mar 10, 2013

OK I added tests using def x(): pass technique and remove the duplicated tests from regression.py.

@davidhalter
Copy link
Owner

After we sorted the whole stuff with testing out, you should just rebase/merge here and then we'll look at this.

@tkf
Copy link
Contributor Author

tkf commented Mar 10, 2013

What do you mean? Should we wait for another merge? Anyway I just rebased. It is easier to do when the diff is small.

davidhalter added a commit that referenced this pull request Mar 11, 2013
Fallback to callee definition when definition not found (fixes #131)
@davidhalter davidhalter merged commit fb0b8b0 into davidhalter:dev Mar 11, 2013
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.

2 participants