Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
HTTP Utils and PathBuilder mods #85
base: main
Are you sure you want to change the base?
HTTP Utils and PathBuilder mods #85
Changes from 16 commits
10ea6d3
0294b56
dc40921
cbb6673
8f7980a
46ee928
d65e86b
726f89a
c68abcb
614f5e8
925b875
bde25d8
d794fb4
750e616
d95a8e8
3be5383
e198139
2f16100
6d2751f
17b469d
21e0af1
abbfc16
73965fc
d0ba6e6
2f2ad5a
9244a61
6df08bd
153ff2e
d8057ec
4677e0b
42bbd28
c7bba8c
6cbd38d
3690410
2fed8b8
36588ed
d7636bf
ea32f26
7514ad3
dedbc45
de06de3
d054eeb
14fcb10
f23a801
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to rename 'aws_ls' to just 'ls' vs having 'ls' call 'aws_ls'. Of course it will no longer be a static method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have to disable linter we want to have as small of a scope as possible. So 'broad-exception-caught' should be for the specific line of code. I'm guessing that the 'duplicate-code' is because this module is very similar to the aws_utils.py, if that is the case maybe we should make a recourse_utils.py for the common and both aws_utils and http_utils extend that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised the possibility of an interface initially, many of the calls made by DAS would be the same for http and aws, at this point it can probably wait for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the refactor we don't need to disable duplicate code, and broad exception should be applied directly to the except and not globally, and ideally we want to specify the specific exception(s) that might happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to incorporate this exception handling in the new implementation of path builder, thanks for finding/fixing. Is there a test that exercises this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My test was running it against the MRMS http feed so no.