Conversation
README.md
Outdated
| ```zsh | ||
| cd my_package | ||
| datacustomcode scan ./payload/entrypoint.py | ||
| datacustomcode zip --path ./payload |
There was a problem hiding this comment.
deploy already runs zip, doesn't it?
There was a problem hiding this comment.
yes, it does. this line is optional.
update: addressed in eb51f58
src/datacustomcode/scan.py
Outdated
| if not dataspace_value or ( | ||
| isinstance(dataspace_value, str) and dataspace_value.strip() == "" | ||
| ): | ||
| logger.error( |
There was a problem hiding this comment.
Should this be a warn maybe?
| raise ValueError( | ||
| f"dataspace must be defined in {config_json_path}. " | ||
| f"Please add a 'dataspace' field to the config.json file." | ||
| ) |
There was a problem hiding this comment.
The diff between missing, null, or empty string is subtle in my mind. It might be simpler to just default to "default" value for all of those cases, instead of raising an exception specifically if it's missing?
There was a problem hiding this comment.
as per our internal follow-up discussion, I'm planning to go with throwing an error when dataspace is excluded by customer explicitly for now. I plan to modify this behavior in case there is feedback indicating some need to handle this differently.
tests/test_scan.py
Outdated
| }, | ||
| }, | ||
| ) | ||
| def test_rejects_empty_dataspace(self): |
There was a problem hiding this comment.
Nit: should this be test_overwrites_empty_dataspace?
tests/test_scan.py
Outdated
| }, | ||
| }, | ||
| ) | ||
| def test_rejects_missing_dataspace(self): |
There was a problem hiding this comment.
I got confused by this test name- looks like we're actually testing test_adds_missing_config_file or something like that.
This also makes me think: do we want a test to cover the missing dataspace case? Currently, the PR is a raising a ValueError, but I'm suggesting perhaps we simplify that.
2 participants
No description provided.