Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

On redefinition of function, give a diagnostic message #1273

Merged
merged 15 commits into from
Jul 16, 2019

Conversation

CTrando
Copy link
Contributor

@CTrando CTrando commented Jul 2, 2019

Note, we skipped this one before because we were worried about collisions with imports, but the function definition code seems to only looks up functions in the local context - so it will only care about other functions declared in the same file as it.

@jakebailey
Copy link
Member

I think there are a few more test cases you're going to want to write, things like:

def foo():
    def foo():
        return 123
    return foo()

foo()

Nested classes, etc.

@CTrando
Copy link
Contributor Author

CTrando commented Jul 2, 2019

Aha, thanks, I was wondering what kind of cases to test beyond this.

@CTrando
Copy link
Contributor Author

CTrando commented Jul 11, 2019

For testing:

  • Functions defined in the same scope

  • Nested functions defined in the same scope

  • Methods defined in the same class - different parameters etc.

  • Functions defined in other modules and imported into global scope doesn't given diagnostics

@MikhailArkhipov
Copy link

Resolve conflicts and LGTM.

@MikhailArkhipov MikhailArkhipov merged commit 8157ede into microsoft:master Jul 16, 2019
jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 2019
* Doing preliminary function redefinition check, need to add testS

* Fixing up error message and cleaning up code

* Adding some light tests

* Reordering

* Adding some more tests on function redefinition

* Switching to nicer syntax in SymbolCollector

* Renaming test files

* Adding redefined tests when importing as well

* handling properties too

* Switching to switch statement

* Minor syntax change

* Unused import

* Switching type out for interface
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants