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

Merge starbase/main #145

Merged
merged 104 commits into from
Jun 15, 2023
Merged

Merge starbase/main #145

merged 104 commits into from
Jun 15, 2023

Conversation

tigarmo
Copy link
Collaborator

@tigarmo tigarmo commented Jun 6, 2023

This PR integrates craft-cli with the current "state of the art" implemented by Starbase. It's a huge changeset because it's a merge of two distinct repositories, but the idea is that future merges will be easier/less painful.

The general strategy was this: First I merged starbase/main into this branch, and for the conflicts I preferred starbase's version for most cases, unless the fix was very straightforward. This merge is commit 13c852f.

After that, I added commits to migrate craft-cli's way of doing things to starbase's. For example, the first commit after the merge removes setup.py and setup.cfg and adds the config to pyproject.toml. This migration was done manually and I inspected/tested the resulting wheel, etc. The next commits add fixes for the individual linters, coverage, and a bigger one for the docs. Then I removed the files that are no longer needed (like the Makefiles).

Hopefully, the task of reviewing per-commit starting at that merge (or the one right after it, since it's hard to review the merge) won't be too onerous because each commit is reasonably self-contained; if it is, tell me and I can see about opening multiple PRs.

lengau and others added 30 commits January 19, 2023 19:03
Most static checks don't need the code to be installed at all, but type checking can require the code and its dependencies.
tools: Make typing use an editable install
tests: Ensure coverage gets per-environment filenames
Done the same way as in rockcraft

Co-authored-by: Tiago Nobrega <tiago.nobrega@canonical.com>
docs: Basic sphinx docs
Rather than statically declaring the project to be "starcraft" all over the place, prefer excluding directories.
chore(tools): Make pyproject.toml exclude rather than statically include
chore(deps): update dependency tox to v4.4.2
chore(deps): update release-drafter/release-drafter action to v5.22.0
chore(deps): update dependency tox-ignore-env-name-mismatch to v0.2.0.post2
chore(deps): update dependency pytest to v7.2.1
Ensure that ruff only gets updated by renovate or explicitly in PRs. This will prevent random CI breakages until ruff is stable.
New ignore: Too many arguments on test functions

* chore(lint): Ruff autofixes for new pylint
tigarmo added 7 commits June 7, 2023 15:23
Enabling the too means solving a bunch of linting issues _plus_ type
issues that are uncovered as ruff annotates things like constructors.

This technical debt is tracked in issue #146.
Like ruff, pyright uncovers a whole set of typing errors which should
be addressed in a separate changeset.

This technical debt is recorded in issue #147
The existing docs don't hard-wrap lines, so disable the check for
line-too-long. Fix some other issues.

With this, all lint- environments in tox should be working.
Reduce from 80% to 76%, which is the value currently achieved in the
codebase.
This commit brings craft-cli's documentation into the format and build
process that Starbase imposes. The specific Makefile and requirements
files in docs/ are removed in favor of the tox 'build-docs' and
'autobuild-docs' that use the dependencies from pyproject.toml.

sphinx-build is called *without* treating warnings as errors, because
the auto-generated doc process generates a bunch of warnings about
circular imports and duplicate declarations that should be fixed at
a later date.

Also remove .readthedocs.yml in favor of the starbase-provided
.readthedocs.yaml.
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@245f3b6). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 59a76ee differs from pull request most recent head 710760e. Consider uploading reports for the commit 710760e to get more accurate results

@@           Coverage Diff           @@
##             main     #145   +/-   ##
=======================================
  Coverage        ?   70.49%           
=======================================
  Files           ?        6           
  Lines           ?      983           
  Branches        ?      187           
=======================================
  Hits            ?      693           
  Misses          ?      287           
  Partials        ?        3           

tigarmo added 2 commits June 7, 2023 17:10
- Remove README.rst and HACKING.rst, as they reference Starcraft
- Keep only the correct release-drafter yaml file
@tigarmo tigarmo changed the title Merge starbase Merge starbase/main Jun 7, 2023
@tigarmo tigarmo requested review from lengau and mr-cal June 7, 2023 20:47
@mr-cal mr-cal added the do-not-squash PR should be merged, not squash-merged label Jun 8, 2023
@mr-cal
Copy link
Contributor

mr-cal commented Jun 8, 2023

I added Claudio's do-not-squash label from craft-parts

Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Looks good!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
HACKING.rst Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@sergiusens
Copy link
Collaborator

I removed branch protection rules to allow this to land when ready

@sergiusens
Copy link
Collaborator

I have also disabled automerge to avoid any accidents

Copy link
Contributor

@lengau lengau left a comment

Choose a reason for hiding this comment

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

I'm happy with this if @mr-cal is :-)

@tigarmo
Copy link
Collaborator Author

tigarmo commented Jun 15, 2023

@lengau @mr-cal I added a new commit re-adding pylint linting (in tox form), because without ruff the codebase is missing a "stylistic" linter. We can remove it again once we add ruff

@tigarmo tigarmo marked this pull request as ready for review June 15, 2023 13:01
Since ruff is currently disabled (pending issue #146), temporarily add
pylint so that we have at least some form of "stylistic" linting of
the code. Once we add ruff we can undo this commit, remove .pylintrc
and remove the many "# pylint:" comments on the code itself.
@tigarmo tigarmo merged commit f6350bf into main Jun 15, 2023
@tigarmo tigarmo deleted the merge-starbase branch June 15, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-squash PR should be merged, not squash-merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants