[Review] Request from 'schubi2' @ 'yast/yast-inetd/review_140929_checking_nil'

[schubi2]

Please review the following changes:

• 7fec9ec packaging
• a013b41 checking nil in GetServerBasename

[kobliha]

REQ: The changelog entry is IMO not understandable

[schubi2]

Yes, you are right.

[kobliha]

REQ: Test case is missing, see https://github.com/yast/yast.github.io/blob/master/doc/code-review.md#code-review-rules

[schubi2]

I have added a test case.

[jreidinger]

NP: well, if you have now testsuite I think it is time to make method little bit more obvious what it do like:

def GetServerBasename(server, server_args)
  arg = server == TCPD_BINARY ? server_args : server
  return nil unless arg

  arg = arg.dup # do we really modify arg???

  server_path = result.strip.split(/[ \t]/).first
  return server_path if server_path.empty?

  File.basename(server_path)
end

[schubi2]

Nice ! I have added. Thanks !

[jreidinger]

typo, it should be arg.strip

[jreidinger]

btw test should show it to you ;)

[schubi2]

* marvel * Yes, the testcase should crash here...

[kobliha]

I'm sorry for being quite picky, but I'm still missing simple test for GetServerBasename

context "server binary is tcpd" do
  context "server name is defined in server_args" do
    it "returns server name from server_args" do
  context "server name is not defined in server_args" do
    it "returns empty name" do
context "server binary is not tcpd" do
  it "returns given server name" do

Yes, it's a big test, I know, but tests the API and the change you are doing in GMC phase

[schubi2]

Lukas, yes you are right. That's also the reason why the typo described above has not been found.

[schubi2]

I have added these testcases AND I have found an additional bug :-)

[kobliha]

NTH: one empty line (separate let and context)

[jreidinger]

LGTM

[kobliha]

@schubi2 Will you merge it and close the bug?

[schubi2]

I have moved to review_140929_checking_nil_SLE-12-GA which is based on SLE-12-GA branch