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

refactor entity and xml_payload #3

Merged
merged 1 commit into from Sep 12, 2013
Merged

Conversation

fabianrbz
Copy link
Contributor

First of all, this is an awesome work!
While reviewing this some rafactors came up, I still have to take a look at some parts of the code, I'm not that familiar with Diaspora's federation, but I'm still learning!

I notice that there are some #TODOs in the code, I can work on them if you like, but probably I'm goning to need some help to understand what is needed to be changed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 2b396f9 on fabianrbz:refactor into a707100 on Raven24:master.

@Raven24
Copy link
Owner

Raven24 commented Sep 8, 2013

thank you very much for reviewing! I will look at this in more detail tomorrow, from a first glance it seems really good.
I sometimes fall back into non-ruby patterns, so any feedback regarding code-style is very welcome.

the problem with the #TODOs is, that they are mostly referencing the problems in the current federation implementation, like fields that don't make sense or stuff that is missing. There should be some discussion about how to handle those issues best, also keeping in mind backward-compatibility...

@fabianrbz
Copy link
Contributor Author

@Raven24 I see, I'll try to take a look at the #TODO's when I have more time

Raven24 added a commit that referenced this pull request Sep 12, 2013
refactor entity and xml_payload
@Raven24 Raven24 merged commit 5c308b6 into Raven24:master Sep 12, 2013
@Raven24
Copy link
Owner

Raven24 commented Sep 12, 2013

thank you!
;)

@fabianrbz fabianrbz deleted the refactor branch September 12, 2013 17:44
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.

None yet

3 participants