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

Enable support for non default config section #338

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Enable support for non default config section #338

merged 1 commit into from
Jul 5, 2018

Conversation

pierreluctg
Copy link
Collaborator

This change allow to use other config sections.

can.ini

[default]
interface = neovi
channel = 1

[HS1]
interface = neovi
channel = 1
serial = 456

[HS2]
channel = 2
serial = 123
>>> hs1_bus = can.interface.Bus(config_section='HS1')
>>> hs1_bus.__class__.__name__
'NeoViBus'

>>> hs2_bus = can.interface.Bus(config_section='HS2')
>>> hs2_bus.__class__.__name__
'NeoViBus'

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

The looks like a good idea. But I think the docs should make clear that the default section is always loaded, and the other given section only updates the config from the default section. This feature in general should probably also be mentioned in doc/configuration.rst.

@pierreluctg
Copy link
Collaborator Author

Documentation have been updated.

@pierreluctg
Copy link
Collaborator Author

@felixdivo and @hardbyte do you think that config_section is a good name or should is be something more generic like bus_name?

can/util.py Outdated
@@ -133,6 +134,10 @@ def load_config(path=None, config=None):
A dict which may set the 'interface', and/or the 'channel', or neither.
It may set other values that are passed through.

:param section:
name of the section in config file to read configuration from.
Default (None) is to use the 'default' section
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "(None)" is not required any more.

@felixdivo
Copy link
Collaborator

The only thing missing now are tests. I think load_config() is not tested anywhere in isolation as of now.

@pierreluctg
Copy link
Collaborator Author

@felixdivo, I will add few tests.

@pierreluctg
Copy link
Collaborator Author

@felixdivo, I have added tests to cover this newly added feature. This should be enough for this pull request. However, we should probably add more tests for load_config() in the future.

@felixdivo
Copy link
Collaborator

Yes, you are right. See #345. Thanks for the tests for this new feature.

@felixdivo
Copy link
Collaborator

I would merge this if the tests ran.

@hardbyte Do you think this is a good addition to the library?

@pierreluctg
Copy link
Collaborator Author

I will fix the code and test soon.

@codecov
Copy link

codecov bot commented Jul 3, 2018

Codecov Report

Merging #338 into develop will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #338      +/-   ##
==========================================
+ Coverage    58.52%   58.6%   +0.07%     
==========================================
  Files           54      54              
  Lines         4207    4215       +8     
==========================================
+ Hits          2462    2470       +8     
  Misses        1745    1745
Impacted Files Coverage Δ
can/util.py 72.63% <100%> (+3.4%) ⬆️
can/interface.py 88.4% <100%> (-2.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63d57e2...d6c6ada. Read the comment docs.

@pierreluctg
Copy link
Collaborator Author

can.util.load_file_config unit tests are all good now :)

Copy link
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Hey @pierreluctg this looks like a great addition. Personally I'd name it straight config or perhaps context instead of config_section - the section seems more like an internal detail where the user really just wants to say "use the configuration HS1 or HS2".

@hardbyte hardbyte added this to the 2.3 Release milestone Jul 3, 2018
@felixdivo
Copy link
Collaborator

Merging #338 into develop will increase coverage by 0.06%.
The diff coverage is 100%.

👍 😄

@pierreluctg
Copy link
Collaborator Author

@hardbyte I do agree that config_section was not the best name.

I have renamed config_section to context and made it mode generic and usable by other config_sources if needed in the the future.

@felixdivo felixdivo merged commit 2776c47 into hardbyte:develop Jul 5, 2018
@pierreluctg pierreluctg deleted the config-sections branch August 14, 2018 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants