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

Extend common_statements #160

Merged
merged 7 commits into from Nov 23, 2021
Merged

Conversation

Jasha10
Copy link
Contributor

@Jasha10 Jasha10 commented Nov 21, 2021

This PR is to close #159, adding support for users to write their own common_statements.

Commits:

Checklist

  • Add test cases to all the changes you introduce #d5e38a37
  • Update the documentation for the changes #30e384bf

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1486867596

  • 17 of 17 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 95.437%

Totals Coverage Status
Change from base Build 1371775256: -0.4%
Covered Lines: 251
Relevant Lines: 263

💛 - Coveralls

@Jasha10
Copy link
Contributor Author

Jasha10 commented Nov 21, 2021

Running make update locally failed for me:

$ make update
...
rm requirements-dev.txt
touch requirements-dev.txt
pip-compile -Ur --allow-unsafe requirements-dev.in --output-file requirements-dev.txt
Could not find a version that matches typing-extensions==3.10.0.2,==4.0.0,>=3.10.0,>=3.7.4 (from -c requirements.txt (line 27))
Tried: 3.6.2, 3.6.2, 3.6.2.1, 3.6.2.1, 3.6.5, 3.6.5, 3.6.6, 3.6.6, 3.7.2, 3.7.2, 3.7.4, 3.7.4, 3.7.4.1, 3.7.4.1, 3.7.4.2, 3.7.4.2, 3.7.4.3, 3.7.4.3, 3.10.0.0, 3.10.0.0, 3.10.0.1, 3.10.0.1, 3.10.0.2, 3.10.0.2, 4.0.0, 4.0.0
There are incompatible versions in the resolved dependencies:
  typing-extensions==3.10.0.2 (from -c docs/requirements.txt (line 125))
  typing-extensions==4.0.0 (from -c requirements.txt (line 27))
  typing-extensions>=3.10.0 (from pylint==2.11.1->-r requirements-dev.in (line 33))
  typing-extensions>=3.7.4 (from mypy==0.910->-r requirements-dev.in (line 43))
  typing-extensions>=3.10.0.0 (from black==21.11b1->-r requirements-dev.in (line 47))
Makefile:14: recipe for target 'update' failed
make: *** [update] Error 2

Copy link
Owner

@lyz-code lyz-code left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, it's always lovely to see when developers follow the code style without any need to do any comment.

Your implementation looks wonderful, I just have some minor suggestions for you to evaluate

src/autoimport/model.py Show resolved Hide resolved
src/autoimport/model.py Outdated Show resolved Hide resolved
src/autoimport/services.py Show resolved Hide resolved
@lyz-code
Copy link
Owner

lyz-code commented Nov 22, 2021

Regarding the make update, I know it's broken :(, I have many of my projects broken in the same way, and I'm figuring out a way to manage these cases automatically with autodev, but it's still not there.

The solution of the dependency break is to locate the packages in docs/requirements.in that don't support typing-extensions==4.0.0 see if there is an issue open that monitors why, and add the typing-extensions < 4.0.0 in the setup.py with the comment to the open issue, so we can remove that pin once it's solved.

It's a lot of unrelated work for this PR, so it's fine if you leave it broken

@Jasha10
Copy link
Contributor Author

Jasha10 commented Nov 22, 2021

... locate the packages in docs/requirements.in that don't support typing-extensions==4.0.0.

It looks like pydantic has a pin on typing-extensions==3.10.0.2.

They do not appear to have an open issue about this.

Copy link
Owner

@lyz-code lyz-code left a comment

Choose a reason for hiding this comment

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

Thank you so much for the contribution!

@lyz-code lyz-code merged commit ebca339 into lyz-code:master Nov 23, 2021
@Jasha10 Jasha10 deleted the extend-common_statements branch November 23, 2021 22:20
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.

Extend common_statements
3 participants