zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

[WIP] Add docstrings to each source file and add linting for developer-file-overview.

Open mounilKshah opened this issue 2 years ago • 9 comments

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

mounilKshah avatar Apr 19 '22 12:04 mounilKshah

@zulipbot add "PR needs review"

mounilKshah avatar Apr 19 '22 13:04 mounilKshah

@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"

mounilKshah avatar Apr 21 '22 03:04 mounilKshah

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.

mounilKshah avatar Apr 27 '22 13:04 mounilKshah

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.

Rohitth007 avatar Apr 27 '22 14:04 Rohitth007

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.

zulipbot avatar May 24 '22 02:05 zulipbot

@mounilKshah I have reviewed your code and found that it does not display a success message in one case.

srdeotarse avatar Jun 04 '22 04:06 srdeotarse

it does not display a success message in one case.

I'll add it, thanks for pointing that out.

mounilKshah avatar Jun 04 '22 13:06 mounilKshah

@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.

Rohitth007 avatar Jun 16 '22 08:06 Rohitth007

@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.

neiljp avatar Aug 15 '22 05:08 neiljp

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.

zulipbot avatar Aug 15 '22 22:08 zulipbot

@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:

  1. 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
  2. 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
  3. 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)
  4. 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
  5. 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.

neiljp avatar Aug 28 '22 19:08 neiljp

@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:

neiljp avatar Sep 10 '22 01:09 neiljp