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

Update dependencies #223

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Update dependencies #223

merged 1 commit into from
Jun 8, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jan 1, 2022

Description (e.g. "Related to ...", etc.)

Update dependencies.

I had intended just to pick up the 1.9.0 release of pydantic; but then while I was here, why not also update all the other things too?

I also shuffled mypy configuration so that it all happens in a single file, which seems to me less confusing than spreading it across two (one of them hidden).

And then, because I was double-checking that the sphinx upgrade didn't break anything, I fixed a couple of typos and a missing reference in the docs.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly
  • CONTRIBUTORS.md was updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@dimbleby
Copy link
Contributor Author

dimbleby commented Jan 1, 2022

Looks like python 3.6 and ws 10 don't play together, I'll back that upgrade out.

However python 3.6 is now end of life, you might reasonably consider dropping support for it (and adding 3.10)?

@perrinjerome
Copy link
Contributor

This also seems good , it solves the same problem as #229 and fix a few other small things at the same time ( but I'm an external contributor and I can not merge )

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 7, 2022

Made some more updates, on account of (i) time having passed and (ii) signs of activity in the repository.

  • dropped support for python 3.6, added 3.10, specified this in setup.cfg

  • require pydantic 1.9.1. I'd previously not bumped the minimum version, but now need to do so because Error importing plugin "pydantic.mypy": cannot import name TypeVarDef pydantic/pydantic#3528

    • arguably we only need recent pydantic in dev, this is specifically a problem with the mypy plugin
    • but I expect that it's not particularly controversial to use a recent pydantic anyway
  • bumped other things that have moved on since January

  • (minor reformatting in setup.cfg for consistency)

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 7, 2022

hmm, I'm not sure why the workflow thinks that it wants results from 3.6 tests, given that I've removed those from the definition.

Edit: I tried a spurious re-commit, and closing and re-opening the MR per some stackoverflow advice, but it didn't help.

Another edit: I think that saying which checks are "required" is a setting on the repository, someone with power to do so would need to update the configuration

@dimbleby dimbleby closed this Jun 7, 2022
@dimbleby dimbleby reopened this Jun 7, 2022
@tombh
Copy link
Collaborator

tombh commented Jun 8, 2022

This is great, LGTM. It will also fix #229 and #235. Might it even fix #237 (poorly handled TimeoutError)? Though I did notice that there was one CI timeout with the latest update commit. Curiously it's not mentioned here in the discussion UI, that's confusing. Either way, I'm assured that this PR doesn't trigger any regressions as tracked by the tests.

Would you like to take care of these last small things:

  • Rebase latest master into the branch
  • Tidy up the commits, just a squash is fine, or if you feel like it; 1 commit for the Python version support change, 1 commit for the dependency update, and 1 for the typos
  • Update python-version: ["3.6", "3.7", "3.8", "3.9"] stanza in .github/workflows/ci.yml
  • Commit message bodies briefly explaining the changes. Also a Fixes #229 #235 should auto close those issues

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 8, 2022

python-version is already updated in ci.yml isn't it? what change are you asking for?

you should be able to squash this yourself if you want with the "merge and squash" button, and write whatever summary comment you want

I'm not at the right computer at the moment to do a rebase. Looks like the merge should be clean though, is a linear history particularly important to you? (It looks as though that hasn't historically been the case in this repository).

tombh
tombh previously approved these changes Jun 8, 2022
Fixes #229 #235

Notes from @dimbleby:
I had intended just to pick up the 1.9.0 release of pydantic; but then
while I was here, why not also update all the other things too?

I also shuffled mypy configuration so that it all happens in a single file,
which seems to me less confusing than spreading it across two (one of
them hidden).

And then, because I was double-checking that the sphinx upgrade didn't
break anything, I fixed a couple of typos and a missing reference in
the docs.
@tombh
Copy link
Collaborator

tombh commented Jun 8, 2022

python-version is already updated in ci.yml isn't it? what change are you asking for?

Oh yes! Sorry, I just saw the still "Expected" test runs for Python 3.6 and assumed ci.yml wasn't updated, but I expect that's just Github offering some security by not using CI config updates from external PRs.

I've rebased master, squashed the commits and forced-pushed (I still have a backup copy of the original branch).

Personally I prefer linear history, so will try to encourage it, but understand it's not everybody's preference.

Just waiting for the nod from the rest of the team, then I'll manually merge this to override the unrun 3.6 tests...

@dimbleby
Copy link
Contributor Author

dimbleby commented Jun 8, 2022

per earlier comment: I think that saying which checks are "required" is a setting on the repository, someone with power to do so would need to update the configuration

@tombh
Copy link
Collaborator

tombh commented Jun 8, 2022

That's possible, but I've never seen that setting in the UI. Maybe only the repo owner can see that. The fact that 3.10 tests are running means that it is indeed using this PR's ci.yml, so my previous theory doesn't seem right either. I'm inclined just to manually merge and see if master still complains about unrun 3.6 tests.

@renatav
Copy link
Contributor

renatav commented Jun 8, 2022

See https://github.com/openlawlibrary/pygls/actions/runs/2461835346

Only running 3.7-3.10 and the builds are passing. So looks like a reporting issue. I don't think that there is a specific setting. Let's merge

@dgreisen dgreisen merged commit f6e3b07 into openlawlibrary:master Jun 8, 2022
@dimbleby dimbleby deleted the update-dependencies branch June 8, 2022 17:08
@dimbleby dimbleby mentioned this pull request Oct 18, 2022
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

5 participants