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 broken MQTT Sensor invert option #8972

Merged
merged 2 commits into from Aug 30, 2020

Conversation

bobjacobsen
Copy link
Member

Also small comment updates

@bobjacobsen bobjacobsen added this to the 4.21.2 milestone Aug 27, 2020
@bobjacobsen bobjacobsen self-assigned this Aug 27, 2020
@mergeable
Copy link

mergeable bot commented Aug 27, 2020

Thanks for the PR. Please consider adding a release note in the help/en/releasenotes/current-draft-note.shtml file.

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #8972 into master will increase coverage by 0.00%.
The diff coverage is 50.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master    #8972    +/-   ##
==========================================
  Coverage     49.62%   49.63%            
- Complexity    54029    54066    +37     
==========================================
  Files          4446     4446            
  Lines        399477   399611   +134     
  Branches      58917    58926     +9     
==========================================
+ Hits         198233   198327    +94     
- Misses       181290   181299     +9     
- Partials      19954    19985    +31     
Impacted Files Coverage Δ Complexity Δ
java/src/jmri/Turnout.java 86.66% <ø> (ø) 6.00 <0.00> (ø)
java/src/jmri/TurnoutManager.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
java/src/jmri/implementation/AbstractLight.java 79.01% <ø> (ø) 28.00 <0.00> (ø)
java/src/jmri/jmrit/logix/TrackerTableAction.java 26.94% <ø> (ø) 8.00 <0.00> (ø)
java/src/jmri/jmrix/JmrixConfigPane.java 36.53% <0.00%> (-0.36%) 21.00 <0.00> (ø)
java/src/jmri/jmrix/acela/AcelaNode.java 28.19% <ø> (ø) 36.00 <0.00> (ø)
java/src/jmri/jmrix/acela/AcelaTurnoutManager.java 45.83% <ø> (ø) 5.00 <0.00> (ø)
...c/jmri/jmrix/acela/nodeconfig/NodeConfigFrame.java 63.41% <0.00%> (ø) 13.00 <0.00> (ø)
.../jmrix/acela/serialdriver/SerialDriverAdapter.java 20.00% <ø> (ø) 4.00 <0.00> (ø)
java/src/jmri/jmrix/cmri/serial/SerialTurnout.java 26.31% <0.00%> (+0.27%) 10.00 <0.00> (ø)
... and 108 more

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 74e240e...53bc570. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #8972 into master will increase coverage by 0.00%.
The diff coverage is 50.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master    #8972    +/-   ##
==========================================
  Coverage     49.62%   49.63%            
- Complexity    54029    54066    +37     
==========================================
  Files          4446     4446            
  Lines        399477   399611   +134     
  Branches      58917    58926     +9     
==========================================
+ Hits         198233   198327    +94     
- Misses       181290   181299     +9     
- Partials      19954    19985    +31     
Impacted Files Coverage Δ Complexity Δ
java/src/jmri/Turnout.java 86.66% <ø> (ø) 6.00 <0.00> (ø)
java/src/jmri/TurnoutManager.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
java/src/jmri/implementation/AbstractLight.java 79.01% <ø> (ø) 28.00 <0.00> (ø)
java/src/jmri/jmrit/logix/TrackerTableAction.java 26.94% <ø> (ø) 8.00 <0.00> (ø)
java/src/jmri/jmrix/JmrixConfigPane.java 36.53% <0.00%> (-0.36%) 21.00 <0.00> (ø)
java/src/jmri/jmrix/acela/AcelaNode.java 28.19% <ø> (ø) 36.00 <0.00> (ø)
java/src/jmri/jmrix/acela/AcelaTurnoutManager.java 45.83% <ø> (ø) 5.00 <0.00> (ø)
...c/jmri/jmrix/acela/nodeconfig/NodeConfigFrame.java 63.41% <0.00%> (ø) 13.00 <0.00> (ø)
.../jmrix/acela/serialdriver/SerialDriverAdapter.java 20.00% <ø> (ø) 4.00 <0.00> (ø)
java/src/jmri/jmrix/cmri/serial/SerialTurnout.java 26.31% <0.00%> (+0.27%) 10.00 <0.00> (ø)
... and 108 more

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 74e240e...53bc570. Read the comment docs.

Copy link
Contributor

@elestedt elestedt left a comment

Choose a reason for hiding this comment

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

if those are public or not won't affect functionality so I won't request changes. But if they can't be reached where they are now, moving them to the turnout class might be a good idea.

public final static String closedText = "CLOSED";
public final static String thrownText = "THROWN";
public final static String unknownText = "UNKNOWN";
public final static String inconsistentText = "INCONSISTENT";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these really be reached from scripting? They're static on an anonymous class.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the constants can stay in this file, but moved to the outer class MqttTurnout.

@danielb987
Copy link
Contributor

@bobjacobsen
This PR seems ready to be merged. The only issue is about constants not being accessible from scripts and that is something I can fix in a separate PR if desired.

Should I go forward and merge this?
Should I create a new PR which moves the constants to the outer class MqttTurnout?

@danielb987 danielb987 merged commit acbd663 into JMRI:master Aug 30, 2020
@bobjacobsen
Copy link
Member Author

bobjacobsen commented Aug 30, 2020 via email

@bobjacobsen bobjacobsen modified the milestones: 4.21.2, 4.21.1 Aug 30, 2020
@bobjacobsen bobjacobsen deleted the mqtt-sensor-invert branch September 11, 2021 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants