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

Integration of diaspora-federation gem #5622

Closed
wants to merge 1 commit into from

Conversation

dimaursu
Copy link
Contributor

@dimaursu dimaursu commented Feb 4, 2015

I tried to integrate the gem by keeping the existing interface intact. So far, I can say that this is the status:

  • HCard
  • webfinger
  • Slap
  • EncriptedSlap
  • MagicEnvelope

The last one is not used in the rest of the code, so we can leave the magic envelope as a internal implementation detail in diaspora-federation gem. If you have any ideas on how to move things forward, I'd be glad to hear. Maybe a top-down approach would have been better.

@dimaursu dimaursu changed the title Integration of diaspora-federation library Integration of diaspora-federation gem Feb 4, 2015
@dnsco
Copy link
Member

dnsco commented Feb 4, 2015

I like the direction of this code, but perhaps "Adapters" is too generic a namespace, and something like "Federation" or Diaspora::Federation might yield less confusion.

@jaywink
Copy link
Contributor

jaywink commented Feb 5, 2015

Awesome stuff @dimaursu - this is very important work

@DeadSuperHero
Copy link
Member

This is definitely a step in the right direction.

If you haven't already, I would recommend talking to @Raven24, who maintains the gem. Honestly, moving the gem repo from Raven24/diaspora-federation to diaspora/diaspora-federation might be useful if we want to have people collaborate on it more frequently.

Maybe worth talking about on Loomio?

require 'account_deleter'

# diaspora-foundation adapters
Copy link
Member

Choose a reason for hiding this comment

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

foundation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was reading something by Isaac Asimov. All the blame goes on this guy.

@goobertron
Copy link

It's fantastic that you're working on this, @dimaursu - thanks very much. Good luck with it!

@dimaursu
Copy link
Contributor Author

dimaursu commented Feb 6, 2015

@denniscollective I really don't want the namespacing to 'conflict' with the one used by the library. Also, I want to convey somehow that the files are logic-less, that the heavy lifting is done by diaspora-federation

@DeadSuperHero I think @Raven24 is aware of what I'm doing :-) Also, I don't think loomio is such a good place to do this: just add comments on the code, ideas, etc. right here on Github

A lot of methods have to be stripped out, together with their tests, because most of that is tested in diaspora-federation, there's no point in duplicating those kind of tests. I think we should keep the original contract with the files. I actually broke the HCard contract, but only because it was easy to fix the thing up. If before HCard was a Hash, now it's an object.

Salmon::EncryptedSlap.create_by_user_and_activity(user, activity)

For example, with EncryptedSlap we should keep create_by_user_and_activity method and make sure it works. All the other stuff - we don't care about it.

@dnsco
Copy link
Member

dnsco commented Feb 6, 2015

@dimaursu my worry with adapters as a top level namespace is that it's not very unique so likely to conflict with another gem that reaches for namespace. maybe Federation::Adapters? or Diaspora::Federation::Adapters (it's a lot to type, but less likely to conflict).

@dimaursu
Copy link
Contributor Author

dimaursu commented Feb 8, 2015

@Raven24 How do you think we should use the Entities? It's not clear to me how they fit in the current situation, yet they are necessary for most of the gem functionality. Maybe define them as modules, and include them into the existing diaspora* models?
@denniscollective Federation::Adapters sounds doable.

@Raven24
Copy link
Member

Raven24 commented Feb 8, 2015

@dimaursu the entity classes from the gem should replace all the xml stuff we have in the models now.
So, you could for instance make a generator "Adapter" that spits out an Entitfy for a given Model or just put a method that does that into every model.

require 'diaspora_federation.rb'
require 'adapters/salmon.rb'
require 'adapters/hcard.rb'
require 'adapters/webfinger.rb'
Copy link
Member

Choose a reason for hiding this comment

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

no need for adding '.rb' at the end

@Raven24
Copy link
Member

Raven24 commented Feb 25, 2015

I think you could make this PR about Webfinger only, then let's test it in the wild while we think about how to best approach converting the other areas to using the gem.

@jaywink
Copy link
Contributor

jaywink commented Feb 25, 2015

+1 for tackling smaller bits at a time

@svbergerem
Copy link
Member

Did #6151 supersede this PR?

@SuperTux88
Copy link
Member

Oh, I didn't know about this PR. But yes, I think this is superseded now.

@denschub
Copy link
Member

Uh. Thanks much, @svbergerem. Closing.

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

10 participants