-
Notifications
You must be signed in to change notification settings - Fork 4
chore: apply Copier template #14
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
Conversation
I wonder why CI is not running |
Unless I can confirm that maintainers agree with most of this PR, I will not be able to open another PR for implementation. Do you have any comments? I suppose @nstarman would have the most opinions about this. When do you think you will be able to review it by? Sorry for the impatience. In case the setup is a hassle, I can do most of it as long as you authorize me to do it. (I've been denied many times though.) |
Review in progress. There are a lot of files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @34j for the PR! Overall it's good and the forward momentum is appreciated.
A number of comments about tailoring this PR to this repo.
build-backend = "setuptools.build_meta" | ||
requires = [ "setuptools" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd prefer to keep hatch as the build system.
Repository = "https://github.com/data-apis/array-api-typing" | ||
Changelog = "https://github.com/data-apis/array-api-typing/releases" | ||
|
||
|
||
[build-system] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering changed. Can you please change it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order was changed by pyproject-fmt on its own. It is not my fault. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. Then for ease of review, can you separate that into two commits (pre and post application of pre-commit)? I'm just scrolling up scrolling down repeatedly to try and make sure that the settings that were discussed and set up in prior PRs are preserved.
tests/test_cli.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
src/array_api_typing/cli.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same and we need cli for automatic code generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the code generation?
module = "docs.*" | ||
ignore_errors = true | ||
|
||
[tool.semantic_release] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. I prefer vcs-based version tagging. We had set up hatch-vcs for this reason.
docs/conf.py can read the version out from the toml.
src/array_api_typing/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm. VCS handles this.
src/array_api_typing/main.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you delete these temp files, CI will fail and you will not know if this template works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course we are going to replace them in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simple __init__.py
should suffice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the coverage report would fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those files don't exist yet on main and coverage is computed relative to main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that makes sense. Please do not try to waste my time. Or you may commit by yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave out the docs for now. It's a big project on its own, with an enormous number of degrees of freedom. We should first decide whether we even want docs, and if we do, mkdocs will probably be more in line with all of the other array-api docs (plus I'm biased, because I can't sphinx haha).
As a contributor to the Copier template, I am very scared to make any major changes to the template because I'm not sure it will work. And please note that I think documentation is primarily needed for the “API Reference”. The template utilized sphinx-apidoc etc. to automatically generate API. |
@nstarman I have no idea how Hatch works and have never used it, can you compromise with current Semantic-Release? All releases are automatic, so there should be no need to do anything manually. All versions will be replaced and updated automatically by Semantic-release bot. |
How does semantic release handle backports? Does it require supporting setup.py and setuptools? What about auto-incrementing per-commit during editable dev installs, which is important for testing CD on TestPyPI? |
I don't think backports are needed for such a small package.
I didn't understand this difficult word ...... what does it mean? At leaset dry-run of Semantic-Release will confirm on PR that most parts of release workflow work fine. That should work on this PR as well... Have you disabled CI? |
I think I could have handled about half of the change requests. How about a compromise on this? Perhaps both your method and this template's method are wonderful and well automated, just the principle is slightly different Thanks for your swift review anyway |
Don't spend time on things that are not essential... So as I said, one-sided censorship is taking place. I don't have infinite time. |
Why are you saying that? |
No one is interested in the formatter or build system applied to the project. The types are needed. |
Development workflow is very important for efficient development. So consider this a necessary investment. But what did you mean when you said
...? |
I apologize if I have offended you, but I would like to point out that any further change requests would depend on personal taste rather than being essential, and would destroy all the advantages of the Copier template. Sorry if my English is not good. |
No.
This copier template is also entirely based on personal taste. And much of it is not essential.
I do think that's the crux of the issue. I'm not convinced this template provides all that good a starting point for this type of project. There are a million different copier templates which I've tried and found them useful for different purposes and almost universally only useful as a starting point not to be closely tied to forever. Here, the use of setuptools, ten different config files for systems we aren't using, dunder files including for CLI (why do we need that), etc. So I think the pushback isn't because we are censoring progress, but because we are trying to ensure it's progress and not bloat. The easiest PR to merge would be a minimal one, which this large template is not. I think it's fair to expect a PR to be reviewed. There's 2 ways to make that process take longer. 1) to be doing something impactful that requires a thoughtful review, and 2) to have a large PR. This PR does both. Half my comments are about removing unnecessary files. That would certainly make the Pr smaller, but there's still important considerations like how the package is installed and built, which is worthy of discussion. I appreciate the forward momentum on this project. Let's systematically address the issues through the standard review process and get this merged in. I doubt we will maintain the ability to do |
When the project is installed from a specific commit (eg in editable mode or during CI/CD) the version is unique and greater than the last release. For TestPyPI this is necessary to ensure that TestPyPI recognizes it as a new version. |
@nstarman Yes, I understand what you are saying, but you are being too particular. I don't care about the details the setup. If I were in your shoes, I would want to complain massively about my PR, and I agree with you 100%:
So I think the problem is that the data-apis community is only collegial, you are the maintainer, and you also happen to have not much time to do the setup 🤔, you are still willing to be a "maintainer" (what are you maintaining ?🤔), and the members did not allow to “replace” me as the maintainer. (I seem to be somewhat not included in "we".) Anyway, I'm not an expert in setup and I don't really know what changes you would like to make, so I would like to leave the rest to you 😴. Especially I have never used hatch. Thanks for your swift response. |
The work was completed at https://github.com/34j/array-api instead. Closing this with a demonstration of the maintainers' willingness not to take over this PR. |
Wrong message received. |
@34j We'd be very happy to take a PR adding in the very-useful automatic code generation, although your work in https://github.com/34j/array-api/tree/main/src/array_api_compat might be best done as an official array-api-compat-stubs package! Something to consider. |
I think this is good for now. @jorenham @nstarman
Read and Write
release
) so that we can already do package name parking after this PR is merged