Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an LMSAL-managed GitHub mirror to the list of SSW mirrors for static AIA response/error files, simplifies error table URL construction, and clarifies the AIA response version constant documentation without changing the version used. Sequence diagram for downloading AIA error table using SSW mirrorssequenceDiagram
actor User_script
participant Aiapy_calibrate_utils as Aiapy_calibrate_utils
participant Mirror_GitHub as Mirror_GitHub
participant Mirror_Soho as Mirror_Soho
participant Mirror_Hesperia as Mirror_Hesperia
User_script->>Aiapy_calibrate_utils: request_error_table(version_3)
Aiapy_calibrate_utils->>Aiapy_calibrate_utils: build_urls_from_SSW_MIRRORS("sdo/aia/response/aia_V3_error_table.txt")
Aiapy_calibrate_utils->>Mirror_GitHub: HTTP GET aia_V3_error_table.txt
alt GitHub mirror has file
Mirror_GitHub-->>Aiapy_calibrate_utils: 200 OK, file contents
Aiapy_calibrate_utils-->>User_script: return error_table_data
else GitHub mirror unavailable
Mirror_GitHub-->>Aiapy_calibrate_utils: error
Aiapy_calibrate_utils->>Mirror_Soho: HTTP GET aia_V3_error_table.txt
alt Soho mirror succeeds
Mirror_Soho-->>Aiapy_calibrate_utils: 200 OK, file contents
Aiapy_calibrate_utils-->>User_script: return error_table_data
else Soho mirror fails
Mirror_Soho-->>Aiapy_calibrate_utils: error
Aiapy_calibrate_utils->>Mirror_Hesperia: HTTP GET aia_V3_error_table.txt
Mirror_Hesperia-->>Aiapy_calibrate_utils: response or error
Aiapy_calibrate_utils-->>User_script: error or error_table_data
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
||
| _SSW_MIRRORS = [ | ||
| # Github mirror managed by LMSAL | ||
| "https://raw.githubusercontent.com/LM-SAL/backup-files/refs/heads/aia/static/", |
There was a problem hiding this comment.
I made it the default for now.
There was a problem hiding this comment.
Is there a reason for preferring this over the "real" mirrors? This does have the disadvantage that any updates to those files will not automatically take effect in aiapy, but maybe that isn't necessarily a bad thing.
There was a problem hiding this comment.
None, I can and will switch it back to SSW.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider keeping the error table filename as a single template constant (similar to
AIA_INSTRUMENT_FILE) rather than hardcoding the V3 path inline, so future versions or additional mirrors only require updating one place. - The
VERSION_NUMBERcomment inchannel.pycould be clarified (and possibly reference the underlying source) to explain why V9 exists but is treated as identical to V8, since this affects how future version bumps should be handled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keeping the error table filename as a single template constant (similar to `AIA_INSTRUMENT_FILE`) rather than hardcoding the V3 path inline, so future versions or additional mirrors only require updating one place.
- The `VERSION_NUMBER` comment in `channel.py` could be clarified (and possibly reference the underlying source) to explain why V9 exists but is treated as identical to V8, since this affects how future version bumps should be handled.
## Individual Comments
### Comment 1
<location path="aiapy/__init__.py" line_range="21" />
<code_context>
_SSW_MIRRORS = [
+ # Github mirror managed by LMSAL
+ "https://raw.githubusercontent.com/LM-SAL/backup-files/refs/heads/aia/static/",
"https://soho.nascom.nasa.gov/solarsoft/",
"https://hesperia.gsfc.nasa.gov/ssw/",
</code_context>
<issue_to_address>
**question (bug_risk):** Double-check the interaction of this mirror URL with `urljoin` and the expected directory structure.
Since downstream code does `urljoin(mirror, "sdo/aia/response/...")`, this trailing `static/` will yield URLs like `.../static/sdo/aia/response/...`. Please confirm that this matches the actual layout of the `backup-files` repo (e.g. `static/sdo/aia/response/...`) and, if not, update either the mirror URL or the relative path used with `urljoin` so the joined URLs point to the correct files.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| _SSW_MIRRORS = [ | ||
| # Github mirror managed by LMSAL | ||
| "https://raw.githubusercontent.com/LM-SAL/backup-files/refs/heads/aia/static/", |
There was a problem hiding this comment.
question (bug_risk): Double-check the interaction of this mirror URL with urljoin and the expected directory structure.
Since downstream code does urljoin(mirror, "sdo/aia/response/..."), this trailing static/ will yield URLs like .../static/sdo/aia/response/.... Please confirm that this matches the actual layout of the backup-files repo (e.g. static/sdo/aia/response/...) and, if not, update either the mirror URL or the relative path used with urljoin so the joined URLs point to the correct files.
f951373 to
54f3517
Compare
|
|
||
| _SSW_MIRRORS = [ | ||
| # Github mirror managed by LMSAL | ||
| "https://raw.githubusercontent.com/LM-SAL/backup-files/refs/heads/aia/static/", |
There was a problem hiding this comment.
Is there a reason for preferring this over the "real" mirrors? This does have the disadvantage that any updates to those files will not automatically take effect in aiapy, but maybe that isn't necessarily a bad thing.
Fixes #390
Summary by Sourcery
Add a new LMSAL-managed GitHub mirror to the list of SSW data mirrors and align related file path handling.
New Features:
Enhancements:
Documentation: