fog / fog-libvirt

libvirt provider for fog
MIT License
16 stars 42 forks source link

Fixed port defaulting to -1 #35

Closed td512 closed 7 years ago

td512 commented 7 years ago

Fixed port defaulting to -1 on line 49

mmoll commented 7 years ago

@td512 what's the status here?

td512 commented 7 years ago

@mmoll pretty much waiting for someone with write access to commit my change into fog-libvirt

mmoll commented 7 years ago

@td512 did you see that inline-comment?

plribeiro3000 commented 7 years ago

@td512 Let me give you a code idea of what i'm talking about. It is about readability i'm talking about. See:

This is the original code:

<graphics type='<%= display[:type] %>' <% if display[:port] and !(display[:port].empty?) %>port='<%= display[:port] %>' autoport='no' <% else %>port='-1' autoport='yes' <%end%> <% if display[:listen] and !(display[:listen].empty?)  %> listen='<%= display[:listen] %>'<% end %> <% if display[:password] and !(display[:password].empty?) %>passwd='<%=display[:password] %>'<% end %> />

Thats what i'm expecting:

<% display_type = display[:type] %>
<%
    if display[:port].empty?
        display_port = display[:port]
        autoport = 'no'
    else
        display_port = '-1'
        autoport = 'yes'
    end

    unless display[:listen].empty?
        display_listen = "listen='#{display[:listen]}'"
    end

    unless display[:password].empty?
        display_password = "passwd='#{display[:password]}'"
    end
%>
<graphics type='<%= display_type %>' port='<%= display_port %>' autoport='<%= autoport %>' <%= display_listen %> <%= display_password %> />

Again, this is for readability purposes only. I know that it is a lot more LOC's then the original code but it makes it a thousand times easier to maintain.

As a note i can tell you that i only fully understood your changes after i did break the code into more lines.

EDIT: As another note, the generated XML will be the same. The extra lines are for ruby pre-processing only.

td512 commented 7 years ago

Duly noted. It does look better. Feel free to update my PR (is that a thing yet?)

mmoll commented 7 years ago

37

plribeiro3000 commented 7 years ago

Closed via #37

plribeiro3000 commented 7 years ago

Thanks everyone!