Skip to content

Conversation

jmthomas
Copy link
Member

closes #581

@jmthomas jmthomas requested a review from ryanmelt April 22, 2023 17:22
@codecov
Copy link

codecov bot commented Apr 22, 2023

Codecov Report

Patch coverage: 67.12% and project coverage change: +0.20 🎉

Comparison is base (86a8498) 74.31% compared to head (200685d) 74.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
+ Coverage   74.31%   74.52%   +0.20%     
==========================================
  Files         460      462       +2     
  Lines       28545    28669     +124     
  Branches      588      588              
==========================================
+ Hits        21214    21366     +152     
+ Misses       7237     7211      -26     
+ Partials       94       92       -2     
Flag Coverage Δ
frontend 75.27% <100.00%> (+0.35%) ⬆️
ruby-api 51.47% <63.07%> (+0.51%) ⬆️
ruby-backend 78.56% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s-cmd-tlm-api/app/controllers/tables_controller.rb 24.17% <0.00%> (+2.60%) ⬆️
openc3-cosmos-cmd-tlm-api/app/models/table.rb 100.00% <100.00%> (ø)
...blemanager/src/tools/TableManager/TableManager.vue 78.64% <100.00%> (+4.68%) ⬆️
...b/openc3/tools/table_manager/table_manager_core.rb 88.34% <100.00%> (ø)
openc3/lib/openc3/version.rb 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jmthomas jmthomas changed the title Table lookup matching definitons and then ask user Table lookup matching definitions and then ask user Apr 22, 2023
@jmthomas jmthomas mentioned this pull request Apr 22, 2023
Copy link
Member

@ryanmelt ryanmelt left a comment

Choose a reason for hiding this comment

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

So what happens now if the definition can't be found?

else
head :not_found
end
rescue Exception => e
Copy link
Member

Choose a reason for hiding this comment

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

Do you not still want an overall Exception catch that can provide an error in json?

Copy link
Member

Choose a reason for hiding this comment

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

Applies throughout

Copy link
Member Author

Choose a reason for hiding this comment

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

The default Rails handler sends the exception. I'm not doing anything special and it's already 500. I'll verify but I think I tried that.

@@ -189,7 +191,7 @@ def self.build_json(binary, definition_filename)
end
end
end
json.as_json(:allow_nan => true).to_json(:allow_nan => true)
Copy link
Member

Choose a reason for hiding this comment

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

You probably want at least the as_json call

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved both to the caller but I can leave that half as long as that's still returning a hash.

@jmthomas
Copy link
Member Author

So what happens now if the definition can't be found?

I have a playwright test for this. It pops up a green warning that says it can't be found along with the File Open dialog for you to select the config file.

@jmthomas
Copy link
Member Author

Default error handler:
Screenshot 2023-04-23 at 3 06 53 PM

@jmthomas jmthomas merged commit 677babb into main Apr 24, 2023
@jmthomas jmthomas deleted the table_load branch April 24, 2023 02:03
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.

TableManager Overly Strict Filename to Definitions Matching
2 participants