Closed txtsd closed 1 year ago
Pidgeot
commented
1 year ago I know you're still working on this, but I would much prefer to keep ignoring wildcard-import, since the automatic override of base tkinter classes is kind of the point of tkinter.ttk and explicitly suggested in the documentation: https://docs.python.org/3.11/library/tkinter.ttk.html
By using the wildcard imports, future versions of ttk can provide new and better-looking implementations, usually with no code changes.
txtsd
commented
1 year ago That makes sense. I'll get it done.
If you have more inputs, feel free to suggest even if the PR isn't ready yet.
txtsd
commented
1 year ago @Pidgeot How do you want me to handle invalid-names?
I could rename them all, but you might want to set some of the single letter variables as good names.
Pidgeot
commented
1 year ago @Pidgeot How do you want me to handle
invalid-names?
Sorry it took a while to get back to you on this; holidays kinda got in the way of looking much at this.
I very much dislike the standard rules Pylint uses to detect invalid names: 1 or 2-letter names are fine in many cases, since the name isn't always important, and line length is a factor too. PEP8 doesn't mention name length anywhere that I can see, either. I know you can change that by altering the regexes it uses, but on the whole, I don't think it adds enough value to be worth dealing with.
If you want to address some (or all) of them anyway, feel free to give it your best shot, and ask about any cases you're unsure of. But I genuinely would not mind just leaving it disabled.
Oh, but make sure to leave the docs folder alone. The documentation generator (Sphinx) decided on those names, so they're not really something I'd want to change.
txtsd
commented
1 year ago Sorry it took a while to get back to you on this; holidays kinda got in the way of looking much at this.
No worries. Everyone's on break during holiday season~
I very much dislike the standard rules Pylint uses to detect invalid names: 1 or 2-letter names are fine in many cases, since the name isn't always important, and line length is a factor too. PEP8 doesn't mention name length anywhere that I can see, either. I know you can change that by altering the regexes it uses, but on the whole, I don't think it adds enough value to be worth dealing with.
If you want to address some (or all) of them anyway, feel free to give it your best shot, and ask about any cases you're unsure of. But I genuinely would not mind just leaving it disabled.
Oh, but make sure to leave the docs folder alone. The documentation generator (Sphinx) decided on those names, so they're not really something I'd want to change.
Will do!
txtsd
commented
1 year ago @Pidgeot Should I also avoid flake8 suggestions for docs/conf.py?
I guess what I want to know is will that file be regenerated at any point in the future, rendering any changes ephemeral?
Pidgeot
commented
1 year ago @Pidgeot Should I also avoid flake8 suggestions for
docs/conf.py?I guess what I want to know is will that file be regenerated at any point in the future, rendering any changes ephemeral?
It shouldn't need to. Honestly, I wouldn't be surprised if it will never be changed ever again.
But I'd prefer to leave it alone, so as to keep it as close to the generated version as possible. That will make it easier to pick our modifications out, since we can more easily compare it with a standard generated file.
txtsd
commented
1 year ago It shouldn't need to. Honestly, I wouldn't be surprised if it will never be changed ever again.
But I'd prefer to leave it alone, so as to keep it as close to the generated version as possible. That will make it easier to pick our modifications out, since we can more easily compare it with a standard generated file.
I actually used sphinx-quickstart to generate a new conf.py, and it's very minimal unlike the file we have. I don't think we'll ever be regenerating it since it's just a quickstart template.
Generated conf.py:
# Configuration file for the Sphinx documentation builder.
#
# For the full list of built-in configuration values, see the documentation:
# https://www.sphinx-doc.org/en/master/usage/configuration.html
# -- Project information -----------------------------------------------------
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-information
project = 'PyLNP'
author = 'Michael Madsen (Pidgeot) and collaborators'
copyright = '2023, Pidgeot'
release = '0.14d'
# -- General configuration ---------------------------------------------------
# https://www.sphinx-doc.org/en/master/usage/configuration.html#general-configuration
extensions = []
templates_path = ['_templates']
exclude_patterns = ['_build', 'Thumbs.db', '.DS_Store']
root_doc = 'index_'
# -- Options for HTML output -------------------------------------------------
# https://www.sphinx-doc.org/en/master/usage/configuration.html#options-for-html-output
html_theme = 'alabaster'
html_static_path = ['_static']I think it's okay to hold it up to Python standards along with the rest of the project as long as we follow the Sphinx docs.
Pidgeot
commented
1 year ago Huh, I hadn't realized Peridexis changed as much as he did when he set it up - in that case, go for it.
(I guess it's also possible Sphinx just generates simpler files today compared to then. But it still seems fine.)
txtsd
commented
1 year ago Not sure why I forgot to mark as ready with the last commit, but oh well. It is.
Let me know what you think.
I'm thinking after this is merged, we can adopt ruff in lieu of pylint and flake8. It is SO FAST!
I also want to create a pyproject.toml to keep this project in sync with current python standards.
txtsd
commented
1 year ago @Pidgeot Gentle reminder to review this~
Pidgeot
commented
1 year ago Right, sorry... I was travelling when you last commented, and then forgot all about it by the time I was home to look at it. >.<
The only point I have left at this point is in the .spec file - the manifest string should probably be split by taking the XML structure into account. But I'll just make that change real quick after merging.
txtsd
commented
1 year ago Awesome!
Pidgeot
commented
1 year ago ...it seems to have broken the documentation generation, though:
Traceback (most recent call last):
File "/usr/lib/python3.11/site-packages/sphinx/config.py", line 350, in eval_config_file
exec(code, namespace)
File "/mnt/data1/pylnp/python-lnp/docs/conf.py", line 25, in <module>
from core import lnp
ModuleNotFoundError: No module named 'core'
make: *** [Makefile:55: html] Error 2Will see if I can't fix that real quick.
Pidgeot
commented
1 year ago It's coming from this change: https://github.com/Pidgeot/python-lnp/pull/191/commits/46dd0f93580e64e4fb02c8ab43f78fdb3b4bccda
The import was moved up before the sys.path changes, so of course it won't see the module. That means I'll have to roll back that particular change.
txtsd
commented
1 year ago Oops I didn't try generating docs after the changes.
Should we include pylint in a commit hook or something of the sort? I've recently been using ruff instead. It's MUCH faster and does more than pylint.
EDIT: While we're at it we could also do CI/CD.
Pidgeot
commented
12 months ago I'm not opposed to the idea as such, but I have zero experience with setting that up on GitHub, so I'd need to read up on that first. I'm also not super clear on whether it makes a ton of sense to do full CI/CD when there's no automated testing.
The documentation already kind of works that way, though; a post-commit hook ends up triggering a rebuild and upload of the -dev version, as well as any newly added tags for named versions. That all happens on my home server, and it might be better and/or easier to just expand that instead. But that's something I'd need to spend time looking into.
This should close #189 and close #190 when it's ready.