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 Attribute Access Issue on Card #43

Merged
merged 3 commits into from
Jul 9, 2014

Conversation

kennethzfeng
Copy link
Contributor

Hi,

It turns out that #42 was due to access violation on readonly properties defined in class Card. Initially, I fixed #42 with a dirty fix by allowing other classes direct access to the somehow private variables on aabe9be. However, it's a bad design so I refactored the JSON deserialization so that direct access is limited within its own class (082135d).

Let me know if you want me to make additional improvements.

-KF

member_ids, short_id, list_id, board_id, and description
are readonly properties.

Changed to direct access the underlying variables:
 - idMembers
 - idShort
 - idList
 - idBoard
 - desc
- Refactored deserialization into class method 'from_json' for every class
- Replaced some simple for loops with list comprehensions
@hany55
Copy link

hany55 commented May 7, 2014

@kennethzfeng I've tested your fork + fixes and I'm still getting 400 - Resource unavailable on write operations. Just curious if you still face the issue with this fix?

YYYY-MM-DD was the chosen format due to the original intention
observed in def set_due, which was different from 'self.due' assignment
in Card.fetch.  For practical reasons, YYYY-MM-DD was chosen.

- Due date of Card is in YYYY-MM-DD format.
- Extra test case was added for set_due on a card.
sarumont added a commit that referenced this pull request Jul 9, 2014
Fixed Attribute Access Issue on Card
@sarumont sarumont merged commit 81f93a2 into sarumont:master Jul 9, 2014
card.desc = json_obj.get('desc', '')
card.closed = json_obj['closed']
card.url = json_obj['url']
card.member_ids = json_obj['idMembers']
Copy link
Contributor

Choose a reason for hiding this comment

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

card.member_ids doesn't exist, since in line 513 you use idMembers

@onilton
Copy link
Contributor

onilton commented Jul 16, 2014

I liked how you solved the problem. I fixed the problem with a different strategy in my branch onilton@55d03bf

But I got your point this is bad design. My only problem with the current code right now is that we have different variables that seems to have the same behaviour/properties: like desc and description.

You could use desc to get the value or set the value. With description you only get the value, but you can't set it (@property only).

Maybe the "private" variables could be like card._desc, card._closed, card._idMembers?

Or maybe just get rid of the @property methods =P ?

@onilton
Copy link
Contributor

onilton commented Jul 16, 2014

BTW, there is a typo in (I know it's not your fault)

@property
def member_id(self):
    return self.idMembers

I fixed it in my branch:

onilton@e6cda42

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.

ResourceUnavailable after calling some APIs containing write operations
4 participants