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

Tooling: requirements and CI #732

Closed
wants to merge 9 commits into from

Conversation

Guts
Copy link
Contributor

@Guts Guts commented Jun 17, 2021

Proposal to improve development workflow and documentation.

Split requirements

Project requirements are not well documented and it's quite hard for a new contributor to know what it's required, especially if we develop without make (i.e;. the makefile is not cross-platform).

I propose here to split requirements between development workflow: base, development, documentation, testing, etc.

It's also useful to cleanly set dependencies versions (minimal and maximal).

Github Actions

Trying to move forward after the CircleCI attempt (#689). Github Action (GHA) seems to be a better option (integration with source code, etc.).

I try here to complete this PR #707 with:

  • dependabot configuration tracking outdated dependencies (using split requirements)
  • linter using flake8
  • documentation build: I did not implement the deploy step because I have no idea about where you want to deploy.

@jelmer
Copy link
Member

jelmer commented Jun 21, 2021

Please propose these as separate changes, so we can evaluate and discuss them one by one.

@Guts
Copy link
Contributor Author

Guts commented Jun 25, 2021

Please propose these as separate changes, so we can evaluate and discuss them one by one.

I can't separate the requirements clean up and the GH Actions because these last use the first. If not, it won't work cleanly and too much dependent on a 'way to develop'.

@jelmer
Copy link
Member

jelmer commented Jun 30, 2021

Please propose these as separate changes, so we can evaluate and discuss them one by one.

I can't separate the requirements clean up and the GH Actions because these last use the first. If not, it won't work cleanly and too much dependent on a 'way to develop'.

What I meant was that the GitHub actions can be a follow-up to the PR that adds the requirements.txt file. The latter doesn't require the former.

That said, we already have dependencies in setup.py and you can iirc run "setup.py develop" to install them. You could e.g. use extras for the docs if we don't do that yet.

That doesn't address the JavaScript dependencies, but afaict neither does requirements.txt.

Copy link
Member

@jelmer jelmer left a comment

Choose a reason for hiding this comment

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

See earlier comments.

@Guts
Copy link
Contributor Author

Guts commented Jan 4, 2022

What I meant was that the GitHub actions can be a follow-up to the PR that adds the requirements.txt file. The latter doesn't require the former.

So you suggest that I should split this PR in multiple?

That said, we already have dependencies in setup.py and you can iirc run "setup.py develop" to install them. You could e.g. use extras for the docs if we don't do that yet.

It's a matter of taste and experience. IMH experience, the storage of dependencies in setup.py is less and less used, for a question of readability, the impossibility to use pip install -r without installing the whole package.

Maybe it's time to use pyproject.toml? Still, if you insist, I can add extras, it doesn't matter to me.

@jelmer
Copy link
Member

jelmer commented Jan 4, 2022

What I meant was that the GitHub actions can be a follow-up to the PR that adds the requirements.txt file. The latter doesn't require the former.

So you suggest that I should split this PR in multiple?
I think that would have made the conversation easier and would have allowed us some of the changes within this PR.

That said, we already have dependencies in setup.py and you can iirc run "setup.py develop" to install them. You could e.g. use extras for the docs if we don't do that yet.

It's a matter of taste and experience. IMH experience, the storage of dependencies in setup.py is less and less used, for a question of readability, the impossibility to use pip install -r without installing the whole package.
requirements .

setup.py and requirements.txt serve slightly different purposes. That said, my main objection here is the duplication. As a maintainer of isso, I don't want to have to change more places than I have today when updating dependencies.

Maybe it's time to use pyproject.toml? Still, if you insist, I can add extras, it doesn't matter to me.

I'm not opposed to using pyproject.toml, but how would that address the concerns raised here?

@Guts
Copy link
Contributor Author

Guts commented Jan 4, 2022

I think that would have made the conversation easier and would have allowed us some of the changes within this PR.

You are right. I close here and I will create new PR.

@Guts Guts closed this Jan 4, 2022
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Left a few comments as hints in case you decide to create a new PR.

@@ -0,0 +1,44 @@
name: "📦 Builder"
Copy link
Member

Choose a reason for hiding this comment

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

In any event, in case you decide to create a PR, please keep the emojis out.

@@ -0,0 +1,8 @@
ci-cd:
Copy link
Member

Choose a reason for hiding this comment

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

isso/js (but not isso/js/admin.js, isso/css -> client
isso/**/*.py -> server
(isso/css/admin.css, isso/js/admin.js, isso/templates/admin.html -> admin)

- any: ["package.json", "requirements.txt", "requirements/*.txt"]

documentation:
- docs/**/*
Copy link
Member

Choose a reason for hiding this comment

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

Our label is called doc, but we can change it. Just to keep in mind.

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.

3 participants