>> + def select_package
>> + found = false
>
> This variable ^ doesn't seem to be used anywhere?
>
Oops, I left this variable in refactorization amok. Deleted.
>> --- a/src/app/models/image_descriptor.rb
>> +++ b/src/app/models/image_descriptor.rb
>> @@ -7,14 +7,18 @@ class ImageDescriptor< ActiveRecord::Base
>> #TODO: validations
>>
>> def update_xml_attributes!(opts = {})
>> - doc = ImageDescriptorXML.new(self[:xml])
>> + doc = xml
>> doc.name = opts[:name] if opts[:name]
>> doc.platform = opts[:platform] if opts[:platform]
>> doc.description = opts[:description] if opts[:description]
>> - doc.services = opts[:services] if opts[:services]
>> - doc.packages = opts[:packages] if opts[:packages]
>> - self[:xml] = doc.to_xml
>> - @xml = doc
>> + doc.services = (opts[:services] || []) if opts[:services] or
>> opts[:set_services]
>> + doc.packages = (opts[:packages] || []) if opts[:packages] or
>> opts[:set_packages]
>
> This looks like it could fail if :set_services exists, but
> not :services, same for packages.
>
In this case is used [], there is '||' in assignment:
opts[:services] || []
>> diff --git a/src/app/util/image_descriptor_xml.rb
>> b/src/app/util/image_descriptor_xml.rb
>> index c5ed310..01eb135 100644
>> --- a/src/app/util/image_descriptor_xml.rb
>> +++ b/src/app/util/image_descriptor_xml.rb
>> @@ -18,8 +18,15 @@
>> # also available at http://www.gnu.org/copyleft/gpl.html.
>>
>> require 'yaml'
>> +require 'util/image_descriptor_xml'
>
> Why does this file require itself? ^
>
Should be 'util/repository_manager', fixed.
>>
>> class ImageDescriptorXML
>> +
>> + UNKNOWN_GROUP = 'Individual packages'
>> + SERVICE_PACKAGE_GROUPS = {
>> + 'jboss' => 'JBoss Core Packages'
>> + }
>> +
>
> I am guessing this ^ is a temporary way to only add jboss until other
> services are supported? If so, please add a FIXME, if not, what do we
> need this for?
>
Yes, it's temporary way, added FIXME comment.
>> def services=(services)
>> service_node = get_or_create_node('services')
>> service_node.xpath('.//service').remove
>> - repositories = repository_manager.repositories
>> services.each do |s|
>> snode = Nokogiri::XML::Node.new('service', @doc)
>> service_node<< snode
>> snode.content = s[0]
>> - if repo = repositories[s[0]]
>> - add_service_packages(s[0], repo)
>> + if group = SERVICE_PACKAGE_GROUPS[s[0]]
> I think you want a '==' here instead of '=' ^, unless I am missing
> something.
>
'=' is right. If there is no group for a service,
SERVICE_PACKAGE_GROUPS[s[0]] is nil => "if block" is not executed.
If group exists it's assigned to "group" in test and used inside "if block".
>> + def all_packages_by_group
>> + groups = {}
>> + all_groups = repository_manager.all_groups
>> + packages_by_group.each do |group, pkgs|
>> + if group_all = all_groups[group]
>
> Again, s/'='/'==' ^
>
Similar to above, all_groups is hash, keys are group names.
>> }, 10000);
>> + $(".expand_target_list_button").css('display', 'block');
>> +
>> $(".expand_target_list_button").click(toggle_build_form_visibility);
> This ^ and its related function can be simplified by using bind to hook
> an anonymous function to the button. You can also simply that
> internally using the goggle method and passing the little snip to set
> button text as the #1&2 handlers.
>
> http://api.jquery.com/bind/
> http://api.jquery.com/toggle/
>
Fixed, didn't know about .toggle() function, thanks. Also moved code
into anonymous function. On the other side, I think usage of
non-anonymous function has some pros: code is simpler to read (split on
smaller pieces) and also reuse of code (but it's not this case).
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
