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

Lock pyright version #24

Merged
merged 4 commits into from
Feb 25, 2022
Merged

Conversation

artificial-aidan
Copy link
Contributor

Took a stab at #23. A couple of caveats.

  1. macos-latest recently deprecated python 3.6, so we removed that. macOS-latest workflows will use macOS-11 actions/runner-images#4060
  2. The version.py script got a little messy, would take some input on cleaning that up, or another way to do it
  3. To test this, I set the __pyright_version__ 1 behind, which lets the workflow get triggered and a PR created. See here for an automated PR: [pyright updated to 1.1.224] Update Version artificialinc/pyright-python#9

Open to input on the whole process, but wanted to get something started.

@artificial-aidan
Copy link
Contributor Author

One other thing.

We specifically grabbed the latest version from the npm registry, as that is where the pyright wrapper pulls it from. Didn't want to trigger off of github releases and have npm be out of sync possibly.

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #24 (b8c2d38) into main (0845fe9) will decrease coverage by 1.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   72.63%   71.33%   -1.30%     
==========================================
  Files          12       13       +1     
  Lines         296      314      +18     
==========================================
+ Hits          215      224       +9     
- Misses         81       90       +9     
Impacted Files Coverage Δ
pyright/_version.py 100.00% <100.00%> (ø)
pyright/cli.py 88.46% <100.00%> (-3.85%) ⬇️
tests/test_main.py 100.00% <100.00%> (ø)
pyright/node.py 59.37% <0.00%> (-8.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0845fe9...b8c2d38. Read the comment docs.

Copy link
Owner

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Some nitpicks.

Also if you are okay with giving me write access to your fork then I'd be more than happy to just push the changes myself.

dev-requirements.txt Outdated Show resolved Hide resolved
pyright/cli.py Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
version.py Outdated Show resolved Hide resolved
@RobertCraigie
Copy link
Owner

We also need a mechanism to automatically release the package on PyPi.

Do you have any suggestions?

@artificial-aidan
Copy link
Contributor Author

I saw you have the release.py script. I think calling that but let it take in some credentials would work. You would have to add repo credentials that match the ones you use for pypi.

I could give that a shot on our local repo, and local pypi registry.

@RobertCraigie
Copy link
Owner

Yeah it's more how do we automatically trigger that script?

We could have another workflow that runs on every commit to main checking if the package version has been changed and making a release then.

@artificial-aidan
Copy link
Contributor Author

Yeah it's more how do we automatically trigger that script?

We could have another workflow that runs on every commit to main checking if the package version has been changed and making a release then.

Ah. I think you could just run it every time a push to main happens. Pypi will reject it if you try and re-publlish an existing package

@RobertCraigie
Copy link
Owner

RobertCraigie commented Feb 24, 2022

I think I've found a better solution.

Move all the version updating and setting to it's own file (pyright/_version.py) and re-export those variables in __init__.py, then we can update the release workflow so that it only runs when pyright/_version.py changes.

_version.py

__version__ = '0.0.13'
__pyright_version__ = '1.1.223'

__init__.py

from ._version import (
    __version__ as __version__,
    __pyright_version__ as __pyright_version__,
)

You would then need to:

  • update the version.py script to change _version.py instead of __init__.py
  • update setup.py to read the version from _version.py

and that should be all you have to change to get the package to work

@artificial-aidan
Copy link
Contributor Author

Ok, moved that stuff around, and added a release workflow that will run after the Test workflow on the main branch.

Couple todos, I'm testing it on our in house repo, so there are some things to change before merging it to your branch. (pypi repository url and runner tags)

Where did we land on matching version to pyright version?

@artificial-aidan
Copy link
Contributor Author

from ._version import (
    __version__ as __version__,
    __pyright_version__ as __pyright_version__,
)

Codacy reports the __version__ import as unused. Should I remove that one?

@RobertCraigie
Copy link
Owner

Codacy reports the version import as unused. Should I remove that one?

No I will just tell codacy to ignore the issue. I honestly do not know why I have it enabled for this repo.

@RobertCraigie
Copy link
Owner

Where did we land on matching version to pyright version?

I think matching versions is a good idea.

@RobertCraigie
Copy link
Owner

Let me know when it's ready for review :)

@artificial-aidan
Copy link
Contributor Author

Ok. So I think this PR is ready for a final review.

Actions needed:

  1. Repository secrets for PyPi login. (PYPI_USER, PYPI_PASS)
  2. Merge to main and do a manual workflow run to generate a PR?

Right now I set the version to 0.0.13.post0 and 1.1.223 so it will first publish 0.0.13.post0 and then it should generate a PR for version 1.1.224 of the package when it gets run on the cronjob or manually

Copy link
Owner

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, thank you! :)

version.py Outdated Show resolved Hide resolved
dev-requirements.txt Outdated Show resolved Hide resolved
@RobertCraigie
Copy link
Owner

Thanks again!

@artificial-aidan
Copy link
Contributor Author

@RobertCraigie I'm pretty sure the combination of workflow_run and paths isn't working. I thought I had tested it but maybe it isn't supported.

I was trying to reuse workflows, but it might be easier to just copy over the test.yml steps in to the release.yml, and have it run on a push to main only if pyright/_version.py changes. Basically just have the test job defined in release.yml and test.yml, and make the publish_release job require the test job to run first.

@RobertCraigie
Copy link
Owner

@artificial-aidan That is unfortunate. In that case I think I'll just remove workflow_run from release.yml as all releases will be made through a PR which I will not merge unless tests pass anyway.

Thanks for the heads up.

RobertCraigie added a commit that referenced this pull request Feb 25, 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.

2 participants