Docstring fixes #60
Docstring fixes #60
Conversation
-Remove class property descriptions -Changeto_dict() dictionary samples from json to python code-block
There was a problem hiding this comment.
This looks great to me @santiagogaray . Thank you for doing it. I only have one comment that would be nice to address before the merge if you are able to.
Replacement of lists of lists with lists of tuples.
|
As you can see @chriswmackey I added them also at every situation were we have lists of lists such as face boundaries, face holes, mesh faces, etc. Hope this is the right format! |
There was a problem hiding this comment.
Thank you for pushing the changes, Santiago. The changes you made were correct but there are a few more places that could benefit from the use of tuples in the dictionary instead of lists. I made a note of one of them here as an example. Addressing these cases is optional in this PR and I'm happy to merge this one in if it's easier. Just let me know.
Also, I realize that I forgot to answer your previous question about re-syncing your fork with the master. You can see from the commit log on the master branch that we are trying to keep the log concise and clean with each PR that it is made so that it's easier to know what changes are in previous versions if we end up having to revert or want to look at out old code. You see that I squashed all of the ~20 commits that you made in your first editing of the docs into one commit on master. So that is why you have to re-sync your fork if you want to make new contributions to master from the same fork. As I noted, there are ways to re-sync your fork automatically (without having to delete and re-fork) and learning these automated methods will likely save you some time in the long run but I realize that there are a few steps to setting it up and it might be easier for you to get the hang of it if I show you over a video call at some point. For now, the manual method is perfectly fine and is used by many people as the xkcd comic strip notes.
ladybug_geometry/geometry2d/_1d.py
Outdated
| @@ -34,7 +34,7 @@ def from_dict(cls, data): | |||
| Args: | |||
| data: A python dictionary in the following format | |||
|
|
|||
| .. code-block:: json | |||
| .. code-block:: python | |||
|
|
|||
| { | |||
| "p": [10, 0], | |||
There was a problem hiding this comment.
You can also change situations like this over to use tuples instead of lists. Basically any dictionary where an array is being used to represent a point or vector should preferably be a tuple.
|
I see, I was on the fence on changing those ones. I would like to change
those if you don't mind before we merge. I'm on the road now but will be
able to fix those in a couple of hours.
…On Tue, Oct 15, 2019, 11:34 AM Chris Mackey ***@***.***> wrote:
***@***.**** approved this pull request.
Thank you for pushing the changes, Santiago. The changes you made were
correct but there are a few more places that could benefit from the use of
tuples in the dictionary instead of lists. I made a note of one of them
here as an example. Addressing these cases is optional in this PR and I'm
happy to merge this one in if it's easier. Just let me know.
Also, I realize that I forgot to answer your previous question about
re-syncing your fork with the master. You can see from the commit log on
the master branch
<https://github.com/ladybug-tools/ladybug-geometry/commits/master> that
we are trying to keep the log concise and clean with each PR that it is
made so that it's easier to know what changes are in previous versions if
we end up having to revert or want to look at out old code. You see that I
squashed all of the ~20 commits that you made in your first editing of the
docs into one commit on master. So that is why you have to re-sync your
fork if you want to make new contributions to master from the same fork. As
I noted, there are ways to re-sync your fork automatically (without having
to delete and re-fork) and learning these automated methods will likely
save you some time in the long run but I realize that there are a few steps
to setting it up and it might be easier for you to get the hang of it if I
show you over a video call at some point. For now, the manual method is
perfectly fine and is used by many people as the xkcd comic strip notes
<https://xkcd.com/1597/>.
------------------------------
In ladybug_geometry/geometry2d/_1d.py
<#60 (comment)>
:
> @@ -34,7 +34,7 @@ def from_dict(cls, data):
Args:
data: A python dictionary in the following format
- .. code-block:: json
+ .. code-block:: python
{
"p": [10, 0],
You can also change situations like this over to use tuples instead of
lists. Basically any dictionary where an array is being used to represent a
point or vector should preferably be a tuple.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60?email_source=notifications&email_token=AEUXWZ3M4GGQ4LSE5VCMEKDQOXPI3A5CNFSM4JAUZUVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIAAOXY#pullrequestreview-301991775>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEUXWZ2YYBH4E2566T5TUM3QOXPI3ANCNFSM4JAUZUVA>
.
|
Replacement of lists of lists with lists of tuples.
|
@chriswmackey I think I covered everything this time but happy to keep tweaking things if needed! Thanks for the explanation, I realize now that I would be able to sync before I start my second PR changes. |
|
Looks great, Santiago. Thanks again for doing this. Merged! |
|
🎉 This PR is included in version 1.7.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Thanks @chriswmackey for guiding me to clean up the repo. I ended up doing option 2.
So what I gather is that each time I'm done with a pull request I should delete the fork and start from a fresh fork in my next PR, correct? I was getting conflicts when I tried to merge the master to my fork, but I thought there would be a way to tell Github to just overwrite my fork. Is that possible?
As I said earlier, I made the changes as discussed, all properties descriptions were removed and to_dict() dictionary examples are now python code-blocks.
Happy to address any further comments, otherwise I look forward to the next documentation job.