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

fixed comments in content.py #106

Closed
wants to merge 2 commits into from
Closed

fixed comments in content.py #106

wants to merge 2 commits into from

Conversation

cgoldberg
Copy link
Contributor

  • some comments in content.py erroneously referred to 'read_now' instead of 'buffer_now'
  • fixed some inconsistencies in quoting (in comments)

@@ -353,7 +353,7 @@ def attach_file(detailed, path, name=None, content_type=None,

This is a convenience method wrapping around ``addDetail``.

Note that unless 'read_now' is explicitly passed in as True, the file
Note that unless 'buffer_now' is explicitly passed in as True, the file
Copy link
Member

Choose a reason for hiding this comment

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

buffer_now is defaulted to True, so I think that the sense of the note needs to be inverted: "Note that by default the contents of the file will be read immediately. If buffer_now is False, then ...

@rbtcollins
Copy link
Member

I've one tweak I'd like - if you can do that and rebase I'll be delighted to merge this.

@cgoldberg
Copy link
Contributor Author

@rbtcollins thanks. I made the change you requested.

@thomir
Copy link
Member

thomir commented Sep 21, 2014

Hi,

Sorry for coming to the party late...

I'm a little concerned about the difference bwteen ' and ` (i.e.- single quotes vs backticks). ReST renders works in backticks as monospace font. Personally I'd prefer that we use backticks when referring to types (like Content, ContentType etc), and single quotes when referring to a parameter name (like buffer_now, etc).

...then again, maybe I'm being too picky? Feel free to override me @rbtcollins if you think that's the case :D

In any case, I think @rbtcollins will insist on a rebase to squash this down to a single merge.

Cheers,

@jml
Copy link
Member

jml commented Oct 7, 2014

@thomir I prefer monospace for all identifiers. I also think that in this particular case a little inconsistency won't cause too much harm for people trying to use testtools (see what I did there?)

@jml
Copy link
Member

jml commented Oct 7, 2014

I'm OK for this to be merged.

@thomir
Copy link
Member

thomir commented Oct 16, 2014

I rebased this (to turn it into a single commit), fixed the commit message, and pushed it to master. I'm closing this PR now.

@thomir thomir closed this Oct 16, 2014
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.

4 participants