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

Major Refactor for Python Standardization and CLI Isolation #95

Merged
merged 14 commits into from
Nov 5, 2023

Conversation

asdf8601
Copy link
Contributor

Hey @rosenpin,

First of all, I've got to say - this project is terrific! However, I ran into some challenges, primarily due to the CLI being intermixed with other systems. This aspect somewhat hindered the product's usability for me, which prompted me to make a few adjustments.

My main objective was to ensure that the project strictly adheres to all Python standards - a move that not only retains the existing functionality but also introduces an extra feature. We can now depend on pipx for cross-system project installation.

Synopsis of Changes:

  • I've transitioned the project to a src layout. This strategy helps prevent unexpected path collapses when executing tests and running linters over src, among other benefits.
  • The tests have been relocated from being part of the main source code to the project root.
  • I've replaced the relative imports with absolute ones for more transparency and accuracy.
  • I addressed and rectified several linting errors that emerged in the process.
  • The project has transitioned from using setup.py/setup.cfg to pyproject.toml, making the build process more seamless.
  • I've included a handy Makefile to handle commonly performed tasks.
  • The requirements.txt file has been updated to feature the latest dependencies.
  • I've also introduced a requirements-dev.txt file, which will exclusively contain development dependencies.
  • The Github-action.yml file has been updated in line with the changes mentioned above.

On the whole, these modifications should add more value to your project for its users, including myself! Thanks for offering such an excellent product to the community.

@matclab
Copy link
Collaborator

matclab commented Oct 28, 2023

Hello,

Thank you for the PR !
I don't know if @rosenpin will have time to review it, so I did a first quick reading.

it looks good to me (I still haven't had time to check installation and running in my computer).
However, I don't get the "CLI isolation" thing you're talking about in your first patch ? Could you expand a bit about what was the problem and what you did to correct it ?

@asdf8601
Copy link
Contributor Author

Hey @matclab, thanks for taking the time to look at the PR!

So, about the "CLI isolation" term—yeah, maybe it wasn't the most straightforward way to describe it. Here's what I meant:

Before, the recommended way to install i3-agenda involved installing all the system dependencies globally using sudo pip install .... This could potentially mess up users' systems due to dependency collisions. Alternatively, users could set up their own venv, but that's kind of overkill just to run a CLI tool, right? And even that approach required some tweaks in the source code.

Now, thanks mainly to the absolute imports (and probably others changes as well) I've added, you can actually install i3-agenda in a virtual environment without any changes to the source code. Even better, you can use pipx for an isolated CLI experience.

So, that's what I meant by "CLI isolation"—making it simpler and safer for users to install and use the tool without affecting their entire system.

Hope this clears things up!

@rosenpin
Copy link
Owner

Hello and thank you for the PR
I unfortunately won't have any availability to help with it in the foreseeable future due to some personal issues, but whatever @matclab says goes as far as I'm concerned.

@matclab
Copy link
Collaborator

matclab commented Oct 30, 2023

Hey @matclab, thanks for taking the time to look at the PR!

Hope this clears things up!

Yes, thanks a lot.
I tried installation via modified AUR package using

python -m  build
python -m installer --destdir="$pkgdir" dist/*.whl

and installation works like a charm.

@mmngreco perhaps update the Download url with the last version, and update the Makefile accordingly to also change this line (or perhaps pyproject syntax allow the reuse of the version as a variable in the rest of the pyproject.toml ?) ?

As far as I concerned once this is corrected, we should merge this PR.

Thank you @mmngreco for this welcomed improvement !

@asdf8601
Copy link
Contributor Author

I'd love to see you enjoying it, @matclab!

perhaps pyproject syntax allow the reuse of the version as a variable in the rest of the pyproject.toml ?

Unfortunately, it doesn't...

perhaps update the Download url with the last version, and update the Makefile accordingly to also change this line?

How about using https://github.com/rosenpin/i3-agenda/releases/latest instead? That is, unless you want to explicitly point out each release.

@asdf8601
Copy link
Contributor Author

I will take care of the correct version of typing_extensions. ;-)

@asdf8601
Copy link
Contributor Author

It seems that the latest version of typing-extensions compatible with Python 3.7 is typing-extensions==4.7.1.

https://pypi.org/project/typing-extensions/4.7.1/

I'll make this change, and it should then pass the CI.

@asdf8601
Copy link
Contributor Author

Regarding the failed job 3.10:

Run pytest tests --junitxml=../junit/test-results.xml
Error: An error occurred trying to start process '/usr/bin/bash' with working directory '/home/runner/work/i3-agenda/i3-agenda/./i3_agenda'. No such file or directory

I will look into this; it appears to be a problem with the directories.

@asdf8601
Copy link
Contributor Author

asdf8601 commented Nov 1, 2023

I've tested the GitHub Action with act, and it works locally. So, fingers crossed, it should work now.

@matclab matclab merged commit 65fa0e8 into rosenpin:master Nov 5, 2023
4 checks passed
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