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

all pattern matching ops on ChargingPointType now cover all cases #1941

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Jul 1, 2019

errors were occurring because the legacy ChargingPointTypes were not properly "cased out" in pattern matching over ChargingPointType values.

i also modified the ChargingPointType Regex so that it is "unanchored" and can grab occurrences of the named charging point from a row at any position. that is consistent with the design, but, now that i'm thinking about it, if a user used a reserved ChargingPointType or PricingModel name in their TAZ name, then the regex would find that. maybe we should restrict our parsing to the row/column instead of the entire row.

anyhow, this fixes the bug. perhaps additionally we should add test class(es) for the charging code.

Closes #1940.


This change is Reviewable

@@ -99,6 +99,12 @@ object ChargingPointType {
case ChargingStationCcsComboType2 => 50
case TeslaSuperCharger => 135
case CustomChargingPoint(_, v, _) => v
// legacy charging points (values taken from 2018 BEAM code)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johanneshiry plz double-check the values i put in place for the legacy types, if you don't mind - both AC/DC and the KwH. i just grabbed them from the old implementation. i think it should work as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "worse" thing about the old definitions is, that Level1 / Level2 Charging rate are not fixed defined but more of a range of installed charging capacity. Anyway the values you picked are in range or close to the definitions of Level1/2 charging. So we can keep them for now until the personal ev branch will be merged in.

@REASY
Copy link
Collaborator

REASY commented Jul 1, 2019

@robfitzgerald there is scala formatiing failure:

23:10:19 :checkScalafmt FAILED

Could you please apply formatting? Thanks!

@ghost
Copy link

ghost commented Jul 2, 2019

test!

@robfitzgerald
Copy link
Collaborator Author

@REASY @johanneshiry if you would be so kind to review, that would be rad 🙏

Copy link
Collaborator

@johanneshiry johanneshiry left a comment

Choose a reason for hiding this comment

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

Ignoring the "inconsistency" of this class for now, it's fine to use it temporarily to make it work again - anyway it should be overwritten be the personal ev branch soon

@@ -99,6 +99,12 @@ object ChargingPointType {
case ChargingStationCcsComboType2 => 50
case TeslaSuperCharger => 135
case CustomChargingPoint(_, v, _) => v
// legacy charging points (values taken from 2018 BEAM code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "worse" thing about the old definitions is, that Level1 / Level2 Charging rate are not fixed defined but more of a range of installed charging capacity. Anyway the values you picked are in range or close to the definitions of Level1/2 charging. So we can keep them for now until the personal ev branch will be merged in.

case Level1 => AC
case DCFast => DC
case UltraFast => DC
case NoCharger => AC
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't make sense as a call on NoCharger should throw an IllegalArgumentException but we can keep this anyway for now to make it work.

@REASY REASY merged commit 126016f into develop Jul 3, 2019
@REASY REASY deleted the rjf/#1940-chargingpointtype-completeness-bug branch July 3, 2019 16:07
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.

Cover new and old ChargingPointTypes in all match statements
3 participants