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

1375 refactor machine variables #1394

Merged

Conversation

@pmansukhani
Copy link
Contributor

commented Jul 17, 2019

  • I have moved machine variables to a new class "MachineVariables".
  • Test cases have been refactored.
  • New test cases added to class TestMachineVariables

Pending:

  • 1 test case is failing. I'm not sure why. Some insight would be helpful
  • Squashing of commits: I will do this after fixing the test case.
@jabdoa2

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

Looks good! That test is indeed weird. Looks like it is only failing on windows and Mac. Linux passes. Might be some race. I will have a look.

The linter has some nits:

Messages
========
mpf/core/machine_vars.py
  Line: 58
    pep257: D105 / Missing docstring in magic method
  Line: 61
    pep257: D105 / Missing docstring in magic method
  Line: 64
    pep257: D102 / Missing docstring in public method
mpf/modes/credits/code/credits.py
  Line: 139
    pep8: E128 / continuation line under-indented for visual indent (col 42)
    pep8: E128 / continuation line under-indented for visual indent (col 42)
  Line: 159
    pep8: E128 / continuation line under-indented for visual indent (col 48)
    pep8: E128 / continuation line under-indented for visual indent (col 48)
mpf/platforms/fast/fast_serial_communicator.py
  Line: 130
    pep8: E501 / line too long (126 > 120 characters) (col 121)
mpf/plugins/auditor.py
  Line: 133
    pep8: E501 / line too long (130 > 120 characters) (col 121)
@pmansukhani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Could you share the command to run the linter?

@jabdoa2

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

We use prospector. Use "pip3 install prospector==1.1.6.3" to install it. The latest release has a bug so we have to pin to the latest working version currently. Just run "prospector" inside the repository.

pmansukhani and others added some commits Jul 17, 2019

@jabdoa2

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2019

Python duck typing hid this error. Basically _monitor_machine_vars created machine_var_monitor in MachineController when called. Unit test look good now (at least locally for me)

@jabdoa2 jabdoa2 merged commit 4f77561 into missionpinball:dev Jul 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pmansukhani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

@jabdoa2 Thanks for helping out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.