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

feat: Add enum value fallbacks for backend object parsing #204

Merged
merged 8 commits into from
Sep 25, 2024

Conversation

EmmaSimon
Copy link
Contributor

Summary

Ticket: Allow for fallback enum values in the backend API parsing code

We have a situation currently where the entire alerts stream will crash if we receive any unexpected enum values from the API, and no alerts will be passed to the frontend as long as the alert with the unexpected value is active. Alerts have many possible cause and effect values, and we've had a handful of cases now where we've received values which weren't documented or reflected in the code. It's also conceivable that new values could be added any time.

This adds a parse function in the v3 object parsing macro which accepts a default enum value to fall back to when none of the existing values match.

Tested on the frontend to ensure that fallbacks were passed through properly, and added tests to both the alert tests and util tests.

@EmmaSimon EmmaSimon requested a review from a team as a code owner September 24, 2024 21:26
@EmmaSimon EmmaSimon requested review from KaylaBrady and removed request for a team September 24, 2024 21:26
Copy link

Coverage of commit 5bba10e

Summary coverage rate:
  lines......: 79.5% (1325 of 1667 lines)
  functions..: 69.1% (560 of 810 functions)
  branches...: no data found

Files changed coverage rate:
                                                                         |Lines       |Functions  |Branches    
  Filename                                                               |Rate     Num|Rate    Num|Rate     Num
  =============================================================================================================
  lib/mbta_v3_api/alert.ex                                               |89.3%     28|61.1%    18|    -      0
  lib/util.ex                                                            | 100%     33| 100%     7|    -      0

Download coverage report

Copy link
Member

@boringcactus boringcactus left a comment

Choose a reason for hiding this comment

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

I think I may have girlbossed a little too close to the sun here with these macros - apologies if this took a while to understand enough to change.

I think default-ness is an inherent property of certain types of enum, and not something that can be different depending on the context in which it's being parsed; I'd rather generate something like

def parse_x(x) do
  case x do
    0 -> :a
    1 -> :b
    _ -> :a
  end
end

so that :a is inherently the default x. Generating the rescue CaseClauseError feels suboptimal to me. I'm going to see if there's a non-miserable way to specify defaults in define_enum or if parse_x/2 is unavoidable, but even if parse_x/2 is unavoidable, we can definitely generate a _ -> default clause instead of rescue CaseClauseErroring.

@EmmaSimon
Copy link
Contributor Author

I think I may have girlbossed a little too close to the sun here with these macros - apologies if this took a while to understand enough to change.

I think default-ness is an inherent property of certain types of enum, and not something that can be different depending on the context in which it's being parsed; I'd rather generate something like

def parse_x(x) do
  case x do
    0 -> :a
    1 -> :b
    _ -> :a
  end
end

so that :a is inherently the default x. Generating the rescue CaseClauseError feels suboptimal to me. I'm going to see if there's a non-miserable way to specify defaults in define_enum or if parse_x/2 is unavoidable, but even if parse_x/2 is unavoidable, we can definitely generate a _ -> default clause instead of rescue CaseClauseErroring.

Haha, it took me a while to wrap my head around, but now I have a better (tho still very vague) understanding of macros and the AST now. I was initially trying to implement it so that a default clause was included, but couldn't figure out a way to get that working in parse_x/2, and I was approaching it from the opposite standpoint, that it would be ideal to have it in parse rather than define, to be prepared for cases where we want a different default in different places where you're parsing the same enum type. I think the justification for either approach is mostly theoretical, but setting it at parse seemed more flexible so I went with it. I think if we define the default in define_enum, it will be easier to have a default case, but I was struggling to get that working within parse_x/2. I wouldn't be surprised if that was possible and I just don't understand macros well enough to do it though.

Copy link

Coverage of commit dceaf2d

Summary coverage rate:
  lines......: 79.5% (1328 of 1670 lines)
  functions..: 69.1% (560 of 810 functions)
  branches...: no data found

Files changed coverage rate:
                                                                         |Lines       |Functions  |Branches    
  Filename                                                               |Rate     Num|Rate    Num|Rate     Num
  =============================================================================================================
  lib/mbta_v3_api/alert.ex                                               |89.3%     28|61.1%    18|    -      0
  lib/util.ex                                                            | 100%     36| 100%     7|    -      0

Download coverage report

@boringcactus
Copy link
Member

The only ways I could think of to define the default inherently were even deeper into inadvisable macro crime territory than this code already is, so I'm fine having the default specified at parse time. dceaf2d generates code that looks more like what I'd write by hand - getting the variable name around macro hygiene is the tough part there.

@EmmaSimon
Copy link
Contributor Author

The only ways I could think of to define the default inherently were even deeper into inadvisable macro crime territory than this code already is, so I'm fine having the default specified at parse time. dceaf2d generates code that looks more like what I'd write by hand - getting the variable name around macro hygiene is the tough part there.

Nice, thank you! This looks like how I was initially trying to implement it, but I didn't understand how the variable stuff worked and didn't realize you wouldn't have to actually pass the default value directly through to set it in parse_default_clause, which seemed wrong.

Copy link

Coverage of commit d08a487

Summary coverage rate:
  lines......: 79.5% (1328 of 1670 lines)
  functions..: 69.1% (560 of 810 functions)
  branches...: no data found

Files changed coverage rate:
                                                                         |Lines       |Functions  |Branches    
  Filename                                                               |Rate     Num|Rate    Num|Rate     Num
  =============================================================================================================
  lib/mbta_v3_api/alert.ex                                               |89.3%     28|61.1%    18|    -      0
  lib/util.ex                                                            | 100%     36| 100%     7|    -      0

Download coverage report

@EmmaSimon EmmaSimon merged commit 9441dc5 into main Sep 25, 2024
5 checks passed
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