zulip-terminal
zulip-terminal copied to clipboard
[WIP] Add docstrings to each source file and add linting for developer-file-overview.
What does this PR do?
- This PR adds docstrings to the source files in the codebase
- Add a CI Action which ensures the document matches the expectations from the docstrings
- Generate the document from docstrings
Fixes #892
Tested?
- [x] Manually
- [x] Existing tests (adapted, if necessary)
- [ ] New tests added (for any new behavior)
- [x] Passed linting & tests (each commit)
Commit flow
- first commit: Adds docstrings to the source files
- second commit: Adds linting for developer-file-overview.md
@zulipbot add "PR needs review"
@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"
The commit flow is not what I would expect. It currently represents the way in which you've done the development rather than adding in stages to illustrate the changes. This can take some getting used to.
Yes. As the PR is a WIP, I thought of fixing the commit flow once we reach a mergeable state. I'll make changes to the commit structure to make clearer.
As a suggestion, it would be more readable if you reverse the order of all your function.
Basically, first main()
then lint_files()
then create_file()
and so on.
If you first tell the reader where the function is used and then tell them what the function does, then it's a much more readable.
Heads up @mounilKshah, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main
branch and resolve your pull request's merge conflicts accordingly.
@mounilKshah I have reviewed your code and found that it does not display a success message in one case.
it does not display a success message in one case.
I'll add it, thanks for pointing that out.
@mounilKshah Most of the PR looks good to me :+1: . It would be easier for other people to go through open conversations if you can resolve those have been addressed. I think there are only a few things left. I'll do some testing but most of the review from my side is done. Hope we can merge this soon.
@mounilKshah I see you didn't change the labels, but since you pushed, I wanted to check if you meant this to be ready for review? Did you intend to push a different version than this? This looks like the previous version and doesn't address the comments, so just wanted to check.
Heads up @mounilKshah, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main
branch and resolve your pull request's merge conflicts accordingly.
@mounilKshah This is at a milestone where I'm tempted to merge, but we discussed a few things in the stream after your push, and there are some points above. To summarize, I think the outstanding things are:
- ideally, it would be beneficial to show all issues, rather than just the first, by accumulating the issues and outputting at the end, rather than exiting on assert or failing to index the array
- As part of (1) it likely would fall out naturally that we would use conditionals instead of exceptions, which would be a pleasant side effect
- Re the coloring, the dependency needs adding with care to one of our dev dependencies (typically separate commit to setup.py), and then I think it's worth applying color across various tools in one go. That's not so important, so we can take the last commit or other working code you have and push into another PR - it'd be good to have to build on later (by you or someone else)
- Add a 'do not modify manually' section, as per discussion here https://github.com/zulip/zulip-terminal/pull/1208#discussion_r894330639 This would be similar to the section at the top of hotkeys.md
- I just discovered that adding a file triggers the linter, is fixed by the fixer, but removing the file doesn't trigger the linter - the document is left as it was!
If you can split out (3) into a separate PR, and fix (5), I'd be happy to merge.
I expect (4) might be simple as an extra commit after the linting commit; it's not a functional change, but useful.
While (1) and (2) could improve the linting feedback, it does fail to lint when there's an error, and says what to do to fix it, so I'm less concerned about fixing that - unless you already have worked further on it.
@mounilKshah Thanks for this last update - this seems to function just fine, including with multiple file differences :+1:
I've adjusted some text, switched some parts to fstrings, and updated the variable name as per the comment at https://github.com/zulip/zulip-terminal/pull/1208#discussion_r894316045
The full differences are available via range-diff
as always.
I'm merging now the tests are done :tada: