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

Added documentation and type-hinting to base element class. #356

Merged
merged 11 commits into from
Feb 13, 2023

Conversation

washad
Copy link
Contributor

@washad washad commented Feb 10, 2023

No description provided.

@washad
Copy link
Contributor Author

washad commented Feb 10, 2023

Note, some of the changes are artifacts of running the "black" formatting tool before submitting my code.

@falkoschindler
Copy link
Contributor

Thanks a lot for this pull request, @washad!
The diff is quite large and we'd like to fix some issues (mainly with linting) before merging.

  • We use autopep8 with a maximum line width of 120 (see .vscode/settings.json). It would be great if you could setup your IDE (PyCharm, I guess) the same. And maybe we can add a similar configuration file like VSCode's settings.json to the repo, so that everybody else with PyCharm automatically uses the same settings.
  • We prefer single quotes. PEP8 doesn't give a preference and we simply chose "'". (For docstrings triple double quotes are recommended. That's right in your code.)
  • There's a ... note:: marker in line 249 (three dots). As far as I can tell this is not valid RST.
  • I think VSCode doesn't show RST notes because they represent additional information that is not important enough for the small popup window. I'm not sure. But maybe we use them more sparingly or not at all.

You also started to improve type-hinting, which is somewhat independent of the documentation and might deserve its own PR with its own dedicated space for discussion. But here are my thoughts so far:

  • We agreed that to_dict deserves an underscore since it's not for public use. And within NiceGUI it's only used twice right before sending it to the client via SocketIO. Therefore I don't see the value of type-annotating its return type more then Dict. An ElementAsDict in a separate file needs to be kept in sync and is prone for regression errors.
  • I'm also sceptical towards the event types. In its current form, the auto-completion does not include events from libraries like Quasar or AG Grid (e.g. "cellDoubleClicked"). And it doesn't indicate the possibility for modifiers (e.g. "keydown.shift.x"). It might make sense when individual elements define their own literal strings for the event type by overwriting the on() method. But then we can also leave the arbitrary string for the base implementation and add more specific events in the child classes.

Long story short: Let's concentrate on documentation and fix the formatting differences.

@falkoschindler falkoschindler added this to the v1.1.7 milestone Feb 10, 2023
@falkoschindler falkoschindler added the documentation Improvements or additions to documentation label Feb 10, 2023
@falkoschindler falkoschindler requested review from falkoschindler and removed request for falkoschindler February 10, 2023 11:16
@washad
Copy link
Contributor Author

washad commented Feb 10, 2023

I'll try to have an update this weekend : )

@washad
Copy link
Contributor Author

washad commented Feb 11, 2023

I've made the requested changes, lemme know : )

@falkoschindler falkoschindler merged commit 849e3a5 into zauberzeug:main Feb 13, 2023
@falkoschindler
Copy link
Contributor

Thanks, @washad!
I reviewed it, did some minor changes here and there and merged it into main. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants