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

156 admin code fixes #157

Merged
merged 3 commits into from
Aug 23, 2024
Merged

Conversation

bbean23
Copy link
Collaborator

@bbean23 bbean23 commented Aug 10, 2024

No description provided.

@bbean23 bbean23 self-assigned this Aug 10, 2024
Copy link
Collaborator

@e10harvey e10harvey left a comment

Choose a reason for hiding this comment

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

The sensisit

@@ -66,8 +66,4 @@ def _opencsp_settings_dirs() -> list[str]:
opencsp_settings = configparser.ConfigParser(allow_no_value=True)
opencsp_settings.read(_settings_files)

for section in opencsp_settings.sections():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a nightly example that causes this for-loop to fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm? I don't think I understand what you mean. Are you saying I should try triggering a nightly test against this PR?

@@ -109,7 +109,9 @@ def _is_binary_file(self, file_path: str, file_name_ext: str):
ext = ext.lower()
if ext == ".ipynb":
is_binary_file = True
if self._is_img_ext(ext):
elif ext == ".h5":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no corresponding test update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is already a test for HDF5 files, but not for this specific use case.

What this change fixes is I overlooked the possibility of a very large HDF5 file being parse-able as text, and thus taking a very long time for this function to return. Really what I should do here is check for large files and assume anything larger than N kilobytes is a binary file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated in commit fd5c12d

Copy link
Collaborator

@braden6521 braden6521 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@bbean23 bbean23 force-pushed the 156-admin-code-fixes branch from fd5c12d to 1e17ec1 Compare August 23, 2024 13:18
@bbean23 bbean23 merged commit 293e0db into sandialabs:develop Aug 23, 2024
4 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.

3 participants