Skip to content

feat: updated readme with documentation guidelines (#770)#806

Merged
batpad merged 1 commit intodevelopfrom
feat.api_docs
Jul 9, 2020
Merged

feat: updated readme with documentation guidelines (#770)#806
batpad merged 1 commit intodevelopfrom
feat.api_docs

Conversation

@gulfaraz
Copy link
Copy Markdown
Member

@gulfaraz gulfaraz commented Jul 3, 2020

Addresses #770

Changes

  • Updated README with a section for Documentation

Checklist

Things that should succeed before merging.

  • Updated/ran unit tests
  • Updated CHANGELOG.md

Release

If there is a version update, make sure to tag the repository with the latest version.

@batpad
Copy link
Copy Markdown
Collaborator

batpad commented Jul 3, 2020

@gulfaraz are the newline additions intentional? For the documentation, I prefer to just let the user Word Wrap and not inject line-breaks, but I'm fine with adding newlines to keep lines shorter, just as long as it was intentional.

The documentation itself looks good to me. My hunch is we may want to split this into its own file down the road, there will likely be a few edge cases to explain, documenting parameters that are not defined in FilterSets, or are defined in inherited classes, etc. This is a great start though and looks good to merge to me. Just let know about the newline fixes, I'm okay either way.

@gulfaraz
Copy link
Copy Markdown
Member Author

gulfaraz commented Jul 8, 2020

@batpad I use black and flake8 (for python) and prettier (for everything else) for formatting. They are not intentional for this particular commit but absolutely intentional for all my projects.

I usually set these up as a pre-commit hook so every commit asserts the same formatting style.

I highly recommend checking these out. Let me know if you're interested in making these an integral part of the code workflow.

Also, I can remove the newlines if you would prefer. These formatting details are only preferences.

@batpad
Copy link
Copy Markdown
Collaborator

batpad commented Jul 8, 2020

I highly recommend checking these out. Let me know if you're interested in making these an integral part of the code workflow.

I think @thenav56 had some ideas / did some setup. At some point we do need to enforce consistency across the code-base, just want to do that in a comprehensive, consistent way.

Also, I can remove the newlines if you would prefer. These formatting details are only preferences.

This is fine - let's just coordinate with @thenav56 soon and see when's a good time to make an overall pass through the code-base and then enforce this with a linter or so.

I'm 👍 on merging this @GregoryHorvath it's just a documentation change, but will be great if you can review and merge, thanks!

@GergiH
Copy link
Copy Markdown
Collaborator

GergiH commented Jul 8, 2020

@batpad Yep we have a flake8 config thanks to @thenav56, that's what is hooked into my VSCode too. Keep in mind by default I believe it uses either 79-80 or 100 as a maxlength for the lines, but we have it at 130 for more flexibility (and indentations).

@gulfaraz I agree with Sanjay on the part that this might just get lost between the lines. My opinion would be that we should restructure/clean the whole README a bit (I don't know how relevant that "Deployment" part is anymore) and introduce a "Table of contents" at the beginning. But this should be another ticket, and it looks good to me too for now, cc @batpad

@nanometrenat nanometrenat removed their request for review July 8, 2020 15:11
@thenav56
Copy link
Copy Markdown
Member

thenav56 commented Jul 9, 2020

@batpad @gulfaraz @GregoryHorvath
We could start by adding custom contributing.md.
Also, It will be very helpful for PR authors/reviewers if we could add GitHub integrated tools like codeclimate.
2020-07-09-104009
Also, let's track test coverage. This way we can enforce adding tests.

@batpad
Copy link
Copy Markdown
Collaborator

batpad commented Jul 9, 2020

Thanks so much @gulfaraz @GregoryHorvath @thenav56 -

I have created 3 new issues:

Anyone feel free to take these up at any point, though I feel we will be able more concertedly prioritize these issues around late-August / early-September once we are done with the current round of large feature building.

Thanks for continuing to raise these - it's very useful to have these issues documented and up as tickets, so we can get them onto the project board in the next phase of work.

@batpad batpad merged commit c70802f into develop Jul 9, 2020
@thenav56 thenav56 deleted the feat.api_docs branch August 14, 2020 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants