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

implemented template and catalog syntax #181

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Conversation

paa001
Copy link
Collaborator

@paa001 paa001 commented Dec 19, 2016

#178 issue

Copy link
Member

@etolstoy etolstoy left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@@ -0,0 +1,29 @@
module Generamba
module Error
class Standard < StandardError; end
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this subclass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry. it's really not necessary. fixed

@@ -0,0 +1,51 @@
module Generamba
module Service
class RemotePlugin
Copy link
Member

Choose a reason for hiding this comment

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

I really need comments for classes and methods, otherwise I can't understand the purpose of some classes :)

Copy link
Member

Choose a reason for hiding this comment

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

I see now that it's a generic class for any type of plugin installed from a remote repository. And catalog is a plugin too.
However it'd be great if you'll add comments for new classes. Or maybe a brief description for a pull request with some implementation details.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think, it`s good idea for new service classes. Done

@paa001
Copy link
Collaborator Author

paa001 commented Dec 21, 2016

can i merge this pull request?

@paa001 paa001 merged commit 44d899b into 2.0.0.alpha Dec 21, 2016
@paa001 paa001 deleted the feature/task-178 branch December 21, 2016 14:45
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.

2 participants