-
Notifications
You must be signed in to change notification settings - Fork 41
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
README.md improve wording, formatting #17
Conversation
3c4f81a
to
e2d2a7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes you did, but I am not sure the changes to the code blocks will still work with pydoctests, could you try it?
Also, you will need to add documentation about the download you implemented (in the PR where you are implementing it).
README.md
Outdated
... 'Dark.City.1998.Directors.Cut.BRRip.H264.AAC.5.1ch.Gopo/') | ||
... video = 'Dark.City.1998.Directors.Cut.BRRip.H264.AAC.5.1ch.Gopo.mp4' | ||
... subtitle = 'Dark.City.1998.Directors.Cut.BRRip.H264.AAC.5.1ch.Gopo.srt' | ||
```python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this will work with pydoctest. Did you try it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test anything because there aren't any tests included - to me, the testing instructions in the README read as incomplete example (as if they were started but never finished).
So, am I interpreting this correctly that everyone is supposed to test "manually" (with the code provided) instead of using a ready-made file/class? Wouldn't it make more sense to include that in the repo and then only have people set values for variables? (For their local file names.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, wait. Having never used pydoctest, I thought the bit about testing was wrt a "regular" testing framework, when actually it's specifically for code examples in documentation? Huh, the more you know!
I'll have to read up on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok, based on quickly skimming info on this, it looks like it won't work if it doesn't find >>>
to denote examples for interactive Python sessions...
I never use the interpreter for anything but the simplest tests, definitely not for playing around with libraries, so find code examples given like that very impractical (bad for copy-pasting bits and pieces and harder to read due to formatting). But I don't know if this is maybe an unusual usage pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those pydoctests will not work either because I am almost 100% sure that you don't have the film Dark City downloaded in: "Dark.City.1998.Directors.Cut.BRRip.H264.AAC.5.1ch.Gopo.mp4" :) That's why I mentioned on #18 that the tests should be moved and probably moved out from the README.
pydoctests is nice when you don't need a lot of setup for the tests, at the moment you need mocks I think it kinda miss its point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, saw #18. Not sure what to do with this PR for now. Should the changes wait until other tests are implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kerstin maybe do the changes you proposed but without touching the code snippets (you can change os
for ost
though). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will revert the changes to the code snippets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Personally, I would make/keep them regular code style than interpreter-style though because as said, it makes copy/pasting examples easier, regardless of where one wants to use the code. They'd need some adapting to be even more useful and more readable – which I think they currently aren't all, which has to do with the superfluous punctuation –, like removal of the assert
statements, which are indicative that this is all about testing (written for a computer)... whereas I think a README should be about explaining things (written for humans).)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @kerstin I agree that having documentation that is easy to copy/paste is a good idea; however, since we don't have proper tests yet don't change the formatting of the code here. We can do it after adding the tests somewhere else.
Yeah, I didn't include the instructions for the download part yet because I was waiting for feedback on the overall changes to the README first. I find it easier to work with separate independent changes/commits than to have to go back and untangle one big commit of which only parts are accepted! |
@kerstin I agree with not adding the download help in this PR, but it should be on the other one. |
e2d2a7b
to
640a91c
Compare
Updated! |
Thanks again for your help @kerstin! |
I changed the variable used in the README from
os
toost
(see #16), and then also went and improved wording/spelling and (code) formatting of the rest of the file along with it:python
keyword (for syntax-highlighting on GitHub)