Skip to content

Conversation

@briancline
Copy link
Member

  • Consistent docblock formats
  • Standardize on Sphinx param formatting
  • Word wrap super long lines in README and LICENSE
  • Add some missing doc blocks
  • Add TODO items where doc blocks need explanation
  • Simplify some wording
  • Remove a few needless blank lines
  • Fix some minor comment formatting (PEP8 consistency)

- Consistent docblock formats
- Standardize on Sphinx param formatting
- Word wrap super long lines in README and LICENSE
- Add some missing doc blocks
- Add TODO items where doc blocks need explanation
- Simplify some wording
- Remove a few needless blank lines
- Fix some minor comment formatting (PEP8 consistency)
Copy link
Contributor

Choose a reason for hiding this comment

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

the 'method' param has three valid options: 'all', 'expired', 'valid'

@jasonjohnson
Copy link
Contributor

For cases where we know what the ID represents, something like "zone_id" or "device_id" would be more appropriate.

In the case of the base ID of any generic item in the client, "item_id" doesn't provide any more information than "id". Simply prefixing it with an underscore would avoid the collision. So, "_id".

@jasonjohnson
Copy link
Contributor

However, in almost all cases the existence of Python's id() is irrelevant. When testing the identity of two objects, you'd hope to have a handle on each - not their memory addresses, which id() represents.

The fact that:

id(obj_a) == id(obj_b)

is identical to the far more Pythonic:

obj_a is obj_b

means I won't be heartbroken about this collision.

@briancline
Copy link
Member Author

I was going to address the above concerns in a revision of this pull request -- what changed in the manual merge?

@CrackerJackMack
Copy link
Contributor

Ah, no worries there. Need to push out a release ASAP for someone. @sudorandom removed __format_filter_dict() in b1c09f9 which caused a conflict. The manual merge only dealt with the conflict so we could tag a release.

None of the mentioned concerns have been addressed. Rebase from master and those can be addressed for v2.2.0

@CrackerJackMack CrackerJackMack merged commit 0d54593 into softlayer:master Mar 12, 2013
@briancline
Copy link
Member Author

Ahh, gotcha. Just curious. I'll rebase and prepare some more stuff when I get back in tomorrow.

@briancline briancline deleted the doc-updates branch March 12, 2013 23:43
@briancline
Copy link
Member Author

Regarding the questions around get_thing(thing_id) vs. get_thing(id) in this change, I noticed we were using just id in other places in the code, so tried to bring some consistency to it. I only picked the latter since the former's signature seemed redundant when I read it, given the context that each were in.

If everyone agrees they'd like to stick with the thing_id convention, I'll revert them in the another pull request shortly. Thoughts are welcome.

@CrackerJackMack
Copy link
Contributor

id is fine since the signatures are within a context.

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