RE: Add black and mdformat to CI

[PraxTube]

Closes #7

Failed previous attempt #9

This PR adds a python linter and a markdown linter to the CI. I added black and mdformat. If you want to add something else feel free to suggest something. The CI is also testing if the project can be build, so if there are some problems with the build process the CI will fail.

In order for you to format your files on you local branch run

black .
mdformat .

Note that this will format in-place. If you want to check see if something would be formatted before actually doing anything use the --check flag (works for both).

Reviewer Notes

Given that all files need to be formatted in order for the CI to pass, I had to run both black and mdformat, which is why there are so many changes. So here are the things that are actually new (that have not changed because of a formatter):

• .github/workflow/python-app.yml this contains the github actions instructions
• requirements.txt added black and mdformat
• added .mdformat.toml which contains settings for mdformat

[PraxTube]

Note that the README.md should be updated too. Though I would honestly rework that altogether (given that there were quite a lot of changes both in the back-end and the front-end). Therefore I would do that in a separate PR.

[efJerryYang]

holy..., you can just let me know that you cannot create a branch (I forgot that, my fault), and I can then force push the updated cleaned history to your remote.

[efJerryYang]

Wait, you can absolutely create a new branch on your local machine. I just tested the command git checkout -b tmp on a forked repository and no problem occurred.

[efJerryYang]

Now, can I just force-push to your remote using the commits I have shown to you in #9 ? This can simplify the procedure, but it will replace your remote commits history, I think you do have it on your local machine currently.

Or I can just merge this PR with your only one commit (I guess it is a merge of the others)?

[efJerryYang]

Fine, I will merge it tomorrow morning if no other changes are applied.

[PraxTube]

Ah sorry I explained it a bit poorly. I meant that I cannot create branches on your repo (I can of course create local branches, but I can't push them). In order for me to create a pull request I need to have a branch to work on. Because I cannot create branches on you repo, I need to create a fork and create branches there which I can then use to create PR.

I am not sure if this is the usual way you do things, I am used to simple have one repo where everyone that contributes can create branches and then you simple use these branches to create PR (I don't know if the Github Standard is to use forks, I haven't collaborated with many people on Github yet). But I now know how to use my fork and keep it up to date so this should not be a problem in the future.

Regardless, this PR contains all the changes, written in one commit. So if you don't want to add anything to this PR, it's ready to be merged.

[efJerryYang]

Yes, we commonly will use the fork of a repository to create PR for collaboration on GitHub. This is regarded as a good practice as it ensures the repository's security when collaborating with other developers we are not familiar. Creating branches will generally be the cases for personal use and not recommended for collaboration, as it may cause a great number of branches and other developers are hard to recognize which one should be for them to work on (although there are something that can specify the exact rights to specific branches, it is another topic and a little tiresome to manage). Forking an independent repository of their own would be much clearer, as developers can focus on their forked repositories, branch their repositories for personal maintenance and raise a PR when they want to merge their contributions to the upstream.

I tend to limit the access right to this repository currently, and later if there are more people interested in this repository I would consider adding you or someone else to the "collaborators" to help manage this repository. But now, only you and I are interested in perfecting this CLI tool so I will keep the original access right.

Anyway, thanks for your interest and contributions to this project! You are always welcome to create new PRs and I will review them as soon as possible!

[efJerryYang]

First I add dependencies for full GitHub markdown support for the use of Todos, but the READMEs still look ugly (broken lines, icons alignment at the top of the docs), thus I revert the use of mdformat temporarily. I also revert the changes in demo and old README.

This PR will be merged when a solution to automatic markdown formatting is found.

Update: I only remove the option wrap = 90 in .mdformat.toml for this purpose.

[efJerryYang]

One more thing to be finished before merging: the dependencies inconsistency in pyproject.toml and requirements.txt.

I am not very sure about how to achieve this automatically, and I also have concerns about what you mentioned earlier about the packages in the requirements.txt.

You said in your earlier commit message (which had been squashed into the commit you were creating this PR) that:

It's much better to let the CI build the packages it needs rather than to have them in the requirements.txt.

I only find the "pip install -r requirements.txt" in the python-app.yml, which actually uses the requirements.txt, and not fully understand what you really mean by "rather than to have them in the requirements.txt".

[PraxTube]

Thanks for explaining the workflow on Github in such great detail! Wait you said certainly makes a lot of sense and forking seems like a clean practice. I was pretty unfamiliar with the concept, so I made some mistakes, but I am more familiar with the concept now, so it should work much better in the future.

[PraxTube]

Regarding the broken lines in the markdown files: Can you give me an example were the lines broke? To me they seem fine (it only create one \n if the line was too long, which is still valid and doesn't change the way it gets rendered). However I just tested a bit and also saw that mdformat seems strange in certain situations. The numbering was reset (which is actually intentional). But we can use number = true in the .mdformat.toml file to keep the consecutive numbering.

[PraxTube]

The added dependencies should not be added to pyproject.toml because they are only needed for the developers. The end-user doesn't need them, and therefore they should not be inside pyproject.toml.

It's much better to let the CI build the packages it needs rather than to have them in the requirements.txt.

I only find the "pip install -r requirements.txt" in the python-app.yml, which actually uses the requirements.txt, and not fully understand what you really mean by "rather than to have them in the requirements.txt".

The CI also tests the build process. For that in needs certain packages, which could be installed directly through requirements.txt but I found that this is not ideal, because this would mean that any need developer would automatically need to download them too (in the least cases they would ever need to build the package though). That's why instead of putting them in requirements.txt, the CI is instead going to create them by running python3 -m pip install --upgrade build. This will install all packages needed to create the build.

[efJerryYang]

Yes, you are right about the rendering using mdformat and I was misled by the rendering of vscode markdown support. It truly does not affect the appearance on GitHub (I checked it at the commit branch da30fe6 just now).

[PraxTube]

Ye the only thing that changed was the numbering see here. However we can disable that by setting number = true in the .mdformat.toml file.

[PraxTube]

I pushed some changes. I added the number = true and also added the wrap = 90 again. The changes made to the .md files are purely new lines now, which doesn't change the way things get rendered.

If you want to format the markdowns differently, feel free to change the settings.

[efJerryYang]

I agree with your decision, but one problem leaves here. Do you still use the CommonMark rather than the full support for GitHub? Maybe you need to first install the requirements.txt so that the reformatting later will not change the Todo list (CommonMark would escape the square brackets and the full support would not).

[efJerryYang]

I merged it now.

[PraxTube]

Awesome 👍