-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement teams command refs: #55317 #11
Conversation
37cafab
to
76a8e50
Compare
76a8e50
to
5b3a0cc
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.
love it 😻
some minor things to take account of
lib/cli.rb
Outdated
require_relative 'adapters/ose_adapter' | ||
require_relative 'models/account' | ||
require_relative 'models/team' | ||
require_relative 'models/ose_secret' |
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.
lib/serializers/team_serializer.rb
Outdated
id: data[:id]) | ||
end | ||
|
||
def render_list(team) |
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.
doesn't #render_list belong to team model ?
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.
Hmm it does. But in the model, I simply delegate to this method. This way, you can easily call the method on your instance and the logic is inside the serializer. Do you think that's an unecessary delegation?
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.
Method definition on model is here: https://github.com/puzzle/ccli/pull/11/files#diff-ac0ed614502ed7cfc515046f59dc52e1R17
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 just think this has really nothing to do with the serializer, does it? the serializer is responsible for converting json to data and versa. whereas #render_list renders a list of folders.
In my opinion, this belongs more to the model in your case. Probably it would belong to a view layer. maybe introduce for example presenters and move it over there ?
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.
from wikipedia: The presenter acts upon the model and the view. It retrieves data from repositories (the model), and formats it for display in the view.
eeadb8e
to
122de50
Compare
122de50
to
5844a90
Compare
No description provided.