search

CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Correctly pass one row to make_table #1060

Closed naved001 closed 5 years ago

naved001 commented 5 years ago

otherwise pretty table fails to generate the table.

zenhack commented 5 years ago

What happens without this?

Also, can you add a regression test?

Also, a readability note, though not really part of this PR: it's super weird and hacky the way we're using passing 'Field' as part of field_names; I'd like to make this a little cleaner. That might be as simple as renaming field_names to column_names, so it reads as less of an abuse of the internals.

ianballou commented 5 years ago

When I did a simple test without the square brackets:

print(make_table(field_names=['Field', 'Value'],
                 rows=['Potato', 'corn']))

I got an error: Row has incorrect number of values. Adding the extra brackets fixed this.

(from https://gist.github.com/ianballou/1870d7629a4e501838044e63127891dc)

naved001 commented 5 years ago

What happens without this?

I think @ianballou answered that with his example. It was treating 'Potato' and 'Corn' as distinct rows, so it would think 'P', 'o', 't', 'a', 't', 'o' are 6 values that make up one row and hence the error says 6!=2.

Also, can you add a regression test?

Sure.

Also, a readability note, though not really part of this PR: it's super weird and hacky the way we're using passing 'Field' as part of field_names; I'd like to make this a little cleaner. That might be as simple as renaming field_names to column_names, so it reads as less of an abuse of the internals.

Since pretty table calls it's columns field_names, I decided to call it that. And for the headers I decided to use 'Field' and 'Value' because that's what openstack CLI does when it displays some arbitrary field and value. I am fine with changing it to column_name if it makes it cleaner.

naved001 commented 5 years ago

I don't know if the unit test I added makes sense, it's just testing pretty table.

I think I should simply add the integration test for the rest of the calls that cause network actions.

Thoughts?

Also, the failures seem unrelated but I'll investigate.

zenhack commented 5 years ago

Yeah, I'd say just do the integration test.

naved001 commented 5 years ago

took me long enough to fix the tests haha.

@zenhack @ianballou does this look good now?