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

Add section on documentation #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add section on documentation #19

wants to merge 1 commit into from

Conversation

ewanjones
Copy link

No description provided.

@ewanjones ewanjones force-pushed the documentation branch 4 times, most recently from 067d815 to 832a2ae Compare November 1, 2018 16:29
Copy link
Contributor

@seddonym seddonym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking nice.

A general thought - some of our architectural guidance might be interim / very specific to our project. For example we might talk about the comms module being a special case that doesn't fit into the layers at the moment. It feels to me like that stuff shouldn't be publicly available, the way this style guide is. Not sure what is the best answer is to this.


### Docstrings

Important classes and modules which are intended to be used throughout the whole codebase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth having some guidance about when to include a big docstring and when it's okay to have less. Personally I think it should be possible to look at every function in isolation and have a good understanding about its inputs, outputs and what it's trying to achieve - but that doesn't always mean it needs a docstring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need this section - I think the gist could be captured with an extra sentence in the existing section on docstrings and comments.

Plus, do we really want to suggest such a verbose docstring template - seem OTT to me. Is there an example of a docstring like this already?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seddonym I agree with you, a lot of small functions wouldn't need documenting, I can make that clearer.

@codeinthehole Most of the sections are suggested, but it was more a way of having a defined layout. I tried to ask questions that could help peopler structure their code more cleanly.


### READMEs

Each directory within the domain layer should be modular and intuitive to use. A README can
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Each directory within the domain layer should be modular and intuitive to use. A README can
Each package within the domain layer should be well structured and intuitive to use. A README can

### READMEs

Each directory within the domain layer should be modular and intuitive to use. A README can
optionally be placed in the root module directory and should detail specifics such as:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
optionally be placed in the root module directory and should detail specifics such as:
optionally be placed in the package directory and should detail specifics such as:

Each directory within the domain layer should be modular and intuitive to use. A README can
optionally be placed in the root module directory and should detail specifics such as:

- How is this module laid out?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- How is this module laid out?
- How is this package laid out?


## Documentation

### READMEs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### READMEs
### Domain package READMEs

This section probably belongs in the "General Python" section, next to the existing note on docstrings.


### Docstrings

Important classes and modules which are intended to be used throughout the whole codebase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need this section - I think the gist could be captured with an extra sentence in the existing section on docstrings and comments.

Plus, do we really want to suggest such a verbose docstring template - seem OTT to me. Is there an example of a docstring like this already?

- How might someone use it?
- What are the use cases?
- How to extend behaviour (e.g. adding a new flow handler)
- Who to contact if you run into errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be including names of particular developers in the code base. That kind of thing gets out of date. If we want to do something like that, it might make more sense to have it in a google doc.

@codeinthehole codeinthehole requested review from a team and removed request for a team February 5, 2019 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants