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.

Sign up for GitHub

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

Jump to bottom

Use InterfaceType everywhere #888

Merged
merged 2 commits into from Jul 18, 2019
Merged

Use InterfaceType everywhere #888

merged 2 commits into from Jul 18, 2019

Conversation

jreidinger
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jul 15, 2019 •
edited

Coverage Status

Coverage increased (+0.03%) to 62.049% when pulling 432a8be on use_connection_config_in_builder into 08af131 on network-ng.

@jreidinger jreidinger force-pushed the use_connection_config_in_builder branch from 1c8a6a0 to 06127e7 Compare July 16, 2019 09:05
@jreidinger jreidinger changed the title [WIP] Use InterfaceType everywhere Use InterfaceType everywhere Jul 16, 2019
mchf
mchf reviewed Jul 16, 2019
View reviewed changes
src/lib/y2network/sysconfig/interface_file.rb
def type
:eth
"eth"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is type a string here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, reason is that in this case it can be also read from file, so I use value that can be specified in file as INTERFACETYPE . Or do you think it is bad approach?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slightly misleading in the context of this pr, but makes sense in this case ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I need to change it as there is now basically only two representation short_name as string and InterfaceType as class. So no symbols anymore.

mchf
mchf reviewed Jul 16, 2019
View reviewed changes
test/default_route_test.rb
@@ -101,9 +102,8 @@
subject.Read
subject.current = 0

builder = Y2Network::InterfaceConfigBuilder.new
builder = Y2Network::InterfaceConfigBuilder.for("eth")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well I know that InterfaceConfigBuilder::for handles even strings, I'm just curious if using string here was an intention.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I start on same places using builder = Y2Network::InterfaceConfigBuilder.for(Y2Network::InterfaceType::ETHERNET) but it is incredible long with all that scoping.

@jreidinger jreidinger force-pushed the use_connection_config_in_builder branch from 06127e7 to 432a8be Compare July 17, 2019 05:55
@mchf
Copy link
Member

mchf commented Jul 17, 2019

fine with me

@jreidinger jreidinger mentioned this pull request Jul 17, 2019
Merged
@jreidinger jreidinger merged commit 7698c31 into network-ng Jul 18, 2019
@jreidinger jreidinger deleted the use_connection_config_in_builder branch July 18, 2019 11:50
@imobachgs imobachgs mentioned this pull request Sep 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchf mchf mchf left review comments

@teclator teclator teclator left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jreidinger @coveralls @mchf @teclator