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

Refactor the rest of AutoinstClass #103

Merged
merged 9 commits into from Apr 20, 2015

Conversation

Projects
None yet
2 participants
@imobachgs
Copy link
Contributor

commented Mar 6, 2015

Hi,

This PR is the second part of #101 so it refactors the rest of the AutoinstClass class.

Thanks.

@@ -19,7 +19,10 @@ module Yast
class AutoinstClassClass < Module
include Yast::Logger

attr_accessor :merge_xslt_path

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

To be honest I think it is not good idea to expose it outside and also allow to change this variable. From my POV it is constant and should be used this way. Rspec allows you to clearly overwrite constants if you need.

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 6, 2015

Author Contributor

Yes, it makes sense. I'll fix that.

deep_copy(out)
log.info "Merge command: #{merge_command}"
out = SCR.Execute(path(".target.bash_output"), merge_command, {})
log.info "Merge stdout: #{out["stdout"]}, stderr: #{out["stderr"]}"

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

just my personal preference, but I like if I see in log together command and its output like


out = SCR.Execute(path(".target.bash_output"), merge_command, {})
log.info "Merge command: #{merge_command}"
log.info "Merge stdout: #{out["stdout"]}, stderr: #{out["stderr"]}"

Reason is that when you enable debug logging, then command and its output and stderr are quite hard to match as there is a lot of garbage between.

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 8, 2015

Author Contributor

Fixed.

def Export
deep_copy(@profile_conf)
end

# Configuration Summary
# Return configuration summary

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

Returns

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

and I think it is duplication of text in @return

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 8, 2015

Author Contributor

Fixed. I've also updated other descriptions.

old_classes_map = Convert.to_map(SCR.Read(path('.xml'), compat_class_file))
old_classes = old_classes_map['classes'] || []
old_classes_map = Convert.to_map(SCR.Read(path(".xml"), compat_class_file))
old_classes = old_classes_map["classes"] || []

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

are you sure that previous call cannot return nil? I think if file do nto exists it can be problem and also if xml contain different stuff then map.

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 8, 2015

Author Contributor

This method is called only if /etc/autoinstall/classes.xml exists but you're right: if the file is empty or contains invalid XML, old_classes_map will be nil.

I've added a new test to cover this case.

}

around(:each) do |example|
FileUtils.mkdir(tmp_dir) unless Dir.exist?(tmp_dir)

This comment has been minimized.

Copy link
@jreidinger

jreidinger Mar 6, 2015

Member

in general I recommend to do rm_rf before this call. It have two benefits:

  1. you do not use already messed tmp_dir
  2. if you run single test, you can afterwards inspect content of tmp dir

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 6, 2015

Author Contributor

I didn't thought about that but you're right. Anyway, I'll keep doing the rm_rf after the test is run, isn't it? (so in case that everything goes well the tmp dir is cleared).

This comment has been minimized.

Copy link
@imobachgs

imobachgs Mar 8, 2015

Author Contributor

Fixed.

@jreidinger

This comment has been minimized.

Copy link
Member

commented Apr 20, 2015

LGTM

imobachgs added a commit that referenced this pull request Apr 20, 2015

Merge pull request #103 from imobach/refactor-remaining-autoinstclass
Finish the refactoring of AutoinstClass

@imobachgs imobachgs merged commit ddac5e9 into yast:master Apr 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@imobachgs imobachgs deleted the imobachgs:refactor-remaining-autoinstclass branch Dec 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.