some stuff - Githubissues

bart6114 commented 2 years ago

First and foremost, very cool initiative!

Some nitpicking below:

[x] there is a dataroots pypi account for when you move to prod, ping me and I can add you to the pypi@dataroost.io group
[x] https://github.com/datarootsio/rootsstyle/blob/main/README.md?plain=1#L48: this block is a bit unclear to me. why is it a code block?
[x] why does legend() display to the left of the plot? If that is the rootsstyle default, why do example plots in README.md show the legend on the right side?
[x] You're using both the terms graph and plot interspersed
[x] I think just the poetry add rootsstyle should be sufficient? Perhaps link the word poetry to the poetry website. https://github.com/datarootsio/rootsstyle/blob/30dc3695476cf4e3bcb8a82ed7b2d09e69f14bdf/README.md?plain=1#L31
[x] Under usage I would like to see a simple hello world style plot example, current usage docs already assume people are completely at ease with matplotlib plotting.
[x] I'm not familiar with these URLs but if this is an actual GH PAT it feels like a security concern. I'd say delete the PAT before going public. But also cherry pick away that history from the file as it doesnt really show best-practice efforts towards security.
[x] I'd probably leave this out, feels like this is implied. It has the danger of people fetching the last version of matplotlib while there deps system might specify another one (and I see you're already specifying matpllotlib as a dep for the install anyway)
[x] this feels like a very heavy dev dep, why do you need it? https://github.com/datarootsio/rootsstyle/blob/main/pyproject.toml#L27 Nvm, it's probably for this ipynb in scripts?
[x] feeling wise the ipynb feels like it should be in a /examples dir because the other files under /scripts are actual dev scripts, they're not intended for the end user?
[x] haven't run the code, but this seems like it won't run https://github.com/datarootsio/rootsstyle/blob/30dc3695476cf4e3bcb8a82ed7b2d09e69f14bdf/pyproject.toml#L30
[x] the src files that start with main_ is this a convention? haven't seen it before. i'm being tedious here but i think without the prefix it would be just as clear to the user what these files are.
[x] I would consider using a GH action to convert this nb to a nice HTML page that you can commit to your GH pages branch. That gives you a very nice landing page with examples that you can link to from the readme.
[x] add your release logic for pypi to a gh action that runs only after being tagged by a version (and succesful tests). i'm personally a fan of https://github.com/codacy/git-version, but there are a lot of options there.
[x] think the automated changelog is cool, but I think it would be even more cool if when you tag the repo with a version, that it automatically creates a GH release and that this info you're gathering for the changelog actually ends up in that release description.
[x] think you should be able to specify this in your .flake8 files (the include/exclude dirs i mean), this makes it easier for local devs to reproduce the GH action
[x] for black you can do the same thing in your pyproject.toml

I'll leave it at that 😇 Again, super cool! 🚀

YannouRavoet commented 2 years ago

First and foremost, very cool initiative!

Thanks a lot

[x] there is a dataroots pypi account for when you move to prod, ping me and I can add you to the pypi@dataroost.io group

Will do

[x] https://github.com/datarootsio/rootsstyle/blob/main/README.md?plain=1#L48: this block is a bit unclear to me. why is it a code block?

Indeed, this doesn't need to be a code block. Replaced it by a list, with only code blocks for the code

[x] why does legend() display to the left of the plot? If that is the rootsstyle default, why do example plots in README.md show the legend on the right side?

This is a typo. legend() shows the legend to the right of the graph.

[x] You're using both the terms graph and plot interspersed

I added 'chart' here and there to balance it a bit more ;). Switched everything to plot.

[x] I think just the poetry add rootsstyle should be sufficient? Perhaps link the word poetry to the poetry website. https://github.com/datarootsio/rootsstyle/blob/30dc3695476cf4e3bcb8a82ed7b2d09e69f14bdf/README.md?plain=1#L31

Once the package is on PyPI (instead of TestPyPI) then you no longer will have to add the source to the pyproject.toml. For now, poetry cannot find the package unless you add the repository source. It then by default, looks first in that source and then in PyPI.

[x] Under usage I would like to see a simple hello world style plot example, current usage docs already assume people are completely at ease with matplotlib plotting.

Added an example of a lineplot and barplot.

[x] I'm not familiar with these URLs but if this is an actual GH PAT it feels like a security concern. I'd say delete the PAT before going public. But also cherry pick away that history from the file as it doesnt really show best-practice efforts towards security.

I'm assuming you are talking about the image paths in the README? The reason a URL is used, is because the PyPI page requires images to be linked with URLs. The reason a token is attached is because the github repo is private. Once, the repo is public, the tokens can be removed.

[x] I'd probably leave this out, feels like this is implied. It has the danger of people fetching the last version of matplotlib while there deps system might specify another one (and I see you're already specifying matpllotlib as a dep for the install anyway)

Removed the pip install matplotlib line

[x] haven't run the code, but this seems like it won't run https://github.com/datarootsio/rootsstyle/blob/30dc3695476cf4e3bcb8a82ed7b2d09e69f14bdf/pyproject.toml#L30

Good catch, this is still something from a blogpost I followed for the first publish. It runs, but apparently defining entry scripts is unnecessary so I removed it.

[x] the src files that start with main_ is this a convention? haven't seen it before. i'm being tedious here but i think without the prefix it would be just as clear to the user what these files are.

Fair enough, renamed the files.

[x] this feels like a very heavy dev dep, why do you need it? https://github.com/datarootsio/rootsstyle/blob/main/pyproject.toml#L27 Nvm, it's probably for this ipynb in scripts?

Yes, its really just to be able to run ipynb scripts in VSCode directly.

[x] feeling wise the ipynb feels like it should be in a /examples dir because the other files under /scripts are actual dev scripts, they're not intended for the end user?

[ ] I would consider using a GH action to convert this nb to a nice HTML page that you can commit to your GH pages branch. That gives you a very nice landing page with examples that you can link to from the readme.

Will continue on this tomorrow

[ ] add your release logic for pypi to a gh action that runs only after being tagged by a version (and succesful tests). i'm personally a fan of https://github.com/codacy/git-version, but there are a lot of options there.

Will continue on this tomorrow

[ ] think the automated changelog is cool, but I think it would be even more cool if when you tag the repo with a version, that it automatically creates a GH release and that this info you're gathering for the changelog actually ends up in that release description.

Will continue on this tomorrow

[x] think you should be able to specify this in your .flake8 files (the include/exclude dirs i mean), this makes it easier for local devs to reproduce the GH action

[x] for black you can do the same thing in your pyproject.toml

Moved the config to the .flake8 and pyproject.toml files

I'll leave it at that innocent Again, super cool! rocket

Thanks for all the feedback

YannouRavoet commented 2 years ago

[x] I would consider using a GH action to convert this nb to a nice HTML page that you can commit to your GH pages branch. That gives you a very nice landing page with examples that you can link to from the readme.

I added a Google Colab link in the README. This link will have to be set when the repo is public (as Google Colab has some permission issues with private repositories of datarootsio). I'll also add a GH page once the repo is public (since the GH page is public (unless paid)).

[x] add your release logic for pypi to a gh action that runs only after being tagged by a version (and succesful tests). i'm personally a fan of https://github.com/codacy/git-version, but there are a lot of options there.

I reworked the workflow and added a section on versioning in the README. Basically, pushes to the main branch now automatically update tags and releases based on the commit messages (README for details). This was somewhat tricky, but works really well.

[x] think the automated changelog is cool, but I think it would be even more cool if when you tag the repo with a version, that it automatically creates a GH release and that this info you're gathering for the changelog actually ends up in that release description.

The automated changelog has been improved so I now use the semver generated logs that get created with every release. The changelog is now completely automated, yet you can tweak the existing changelog manually as much as you want :).

datarootsio / rootsstyle

some stuff #12