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

Remove most use of literal lists of simulators. #200

Merged
merged 3 commits into from
Dec 18, 2017

Conversation

jamessynge
Copy link
Contributor

Add pocs/hardware.py, with info about all the types of hardware that could exist, and support for producing the list of simulators. Use this in place of hardcoded lists of simulators.

Supports #134 and #199, but does not fix either one.

could exist, and support for producing the list of simulators.
Use this in place of hardcoded lists of simulators.
@codecov
Copy link

codecov bot commented Dec 17, 2017

Codecov Report

Merging #200 into develop will increase coverage by 0.08%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #200      +/-   ##
===========================================
+ Coverage    81.57%   81.66%   +0.08%     
===========================================
  Files           38       39       +1     
  Lines         2709     2722      +13     
  Branches       342      344       +2     
===========================================
+ Hits          2210     2223      +13     
  Misses         392      392              
  Partials       107      107
Impacted Files Coverage Δ
pocs/utils/config.py 90.47% <100%> (+2.97%) ⬆️
pocs/base.py 96.87% <100%> (-0.19%) ⬇️
pocs/hardware.py 87.5% <87.5%> (ø)

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 b3ca111...37fe55b. Read the comment docs.

@jamessynge jamessynge requested a review from wtgee December 17, 2017 00:52
Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

Much cleaner, thanks!

pocs/hardware.py Outdated
ALL_NAMES = sorted(['camera', 'dome', 'mount', 'night', 'weather'])


def GetAllNames(all_names=ALL_NAMES, without=None):
Copy link
Member

Choose a reason for hiding this comment

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

get_all_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pocs/hardware.py Outdated
def GetAllNames(all_names=ALL_NAMES, without=None):
"""
"""
if without:
Copy link
Member

Choose a reason for hiding this comment

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

If you change the default to be without=list() then you don't need to check for the if and you can just do the one line return statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pocs/hardware.py Outdated
return list(all_names)


def GetSimulatorNames(simulator=None, kwargs=None, config=None):
Copy link
Member

Choose a reason for hiding this comment

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

get_simulator_names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pocs/hardware.py Outdated
Or:
GetSimulatorNames(simulator=simulator, config=self.config)

The reason this function doesn't just take **kwargs as its sole arg is that we need to allow
Copy link
Member

Choose a reason for hiding this comment

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

I don't doubt this is true but seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annoying but true.

pocs/hardware.py Outdated
List of names of the hardware to be simulated.
"""
def ExtractSimulator(d):
if d:
Copy link
Member

Choose a reason for hiding this comment

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

I keep meaning to mention this in other places, but the more pythonic way (EAFP) would be to:

try:
    return d['simulator']
except KeyError:
    return None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided on yet another approach.

pocs/hardware.py Outdated
if d:
return d.get('simulator')
return None
for simulator in [simulator, ExtractSimulator(kwargs), ExtractSimulator(config)]:
Copy link
Member

Choose a reason for hiding this comment

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

Seems odd to reuse simulator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed... and what a stupid idea! Clearly a sign that I added the simulator kwarg after deciding that the for loop variable would be called simulator.

pocs/hardware.py Outdated
Returns:
List of names of the hardware to be simulated.
"""
def ExtractSimulator(d):
Copy link
Member

Choose a reason for hiding this comment

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

extract_simulator

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

Meant to mark Request Changes for the CamelCase names.

@wtgee wtgee merged commit b8f0026 into panoptes:develop Dec 18, 2017
@jamessynge jamessynge deleted the hardware_names branch December 20, 2017 21:20
@wtgee wtgee mentioned this pull request Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants