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

Fix circular import, add test survey action, add interpolated values to calibration files #48

Merged
merged 24 commits into from
Nov 4, 2022

Conversation

jhazentia
Copy link
Member

  • Fixes circular import issues by moving signals.py and removing gps import from scos_actions hardware
  • Adds test survey iq action
  • Adds interpolated values for USRP actions and test actions to the example calibration files

jhazentia and others added 23 commits May 4, 2022 15:52
…dateutil dependency, update dependencies, fix django version for dependabot alert, set scos-actions version 1.0.1
Move signals to scos_actions and ran pre-commit.
Copy link
Member

@aromanielloNTIA aromanielloNTIA left a comment

Choose a reason for hiding this comment

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

Looks good! See a couple small comments. With a refactoring change like this it may also be worth another quick pass through the code (or search-through using your IDE) to check for any references to the changed imports (signals, gps) that might have been overlooked (they wouldn't show in the diffs).

Another thing to consider is that there are a few places here where MockGPS() is passed as a default argument. The MockGPS instance is technically a mutable object. Based on the definition of the MockGPS, I wouldn't expect this to cause any issues as it is currently implemented. However, if the GPS interface is ever expanded and changed significantly in the future, this could lead to unexpected behavior that's hard to track down. A suggested fix would be replacing instances like:

def some_function(parameter=MockGPS()):
    do_something(parameter)
    ...

with

def some_function(parameter=None):
    if parameter is None:
        parameter = MockGPS()
    do_something(parameter)
    ...

scos_actions/hardware/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@aromanielloNTIA aromanielloNTIA left a comment

Choose a reason for hiding this comment

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

All my previous comments have been addressed. I also think the addition of logger warnings for the mock sigan/gps are great additions.

@jhazentia jhazentia merged commit 2cb403c into master Nov 4, 2022
@jhazentia jhazentia deleted the fix_dateutil_dependency branch November 4, 2022 21:51
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.

3 participants