-
Notifications
You must be signed in to change notification settings - Fork 20
Conversation
f7dd59c
to
05a88e2
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.
Thanks for these tweaks @bradenmacdonald , the API is looking much nicer!
I did a full read through everything again since we're fine tuning, and made several suggestions below.
But the code example works as described, so will approve when these changes are made.
tagstore/models/taxonomy.py
Outdated
Add the specified tag to this given taxonomy, and retuns it. | ||
|
||
Will be a no-op if the tag already exists in the taxonomy (case-insensitive), | ||
however the returned (existing) Tag may differ in case. |
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 wasn't able to find a case where the "returned (existing) Tag may differ in case" occurrs.. suggest something like this instead?
If a Tag already exists in the taxonomy with the given name (case-insensitive) with the given parent, then that Tag is returned and no changes are made.
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 wasn't able to find a case where the "returned (existing) Tag may differ in case"
There is a test case showing that? https://github.com/open-craft/blockstore/blob/05a88e283d81f67bb7094df6d7a453ef40ab3890/tagstore/backends/tests.py#L73-L75
I do like your wording though, so I'll use 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.
Ah gotcha.. thanks for that example!
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.
Looks like your change to the wording hasn't make it in yet?
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.
It did make it in in tagstore.py
but I missed taxonomy.py
. Fixed now :)
Thanks @pomegranited, great points. Fixes are all made in aba94c9 |
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.
@bradenmacdonald Thank you for these changes! I think there's just one missing, but ok to merge once it's in 👍
- I tested this by running the API example code from the updated README and checking the results came out as expected.
- I read through the code.
-
I checked for accessibility issuesN/A - Includes great documentation.
Although this pull request is already merged, I've created OSPR-5476 so that we can track it in Jira. There is nothing you have to do. No action is needed from your side. Thanks again for your contribution. |
A few last API improvements to make this tagstore API a bit nicer, before we get too locked in.
Tag.tag
is nowTag.name
which I think is much nicerTag
without constructing it yourself, so I added aget_tag()
methodTaxonomy
class a bit more pythonic by giving it some methods, so now you can dotaxonomy.add_tag('foo')
instead oftagstore.add_tag_to_taxonomy('foo', taxonomy)
Testing instructions:
README.rst
.