Fail immediately and emit a relevant error if column mappings are missing from config.json#595
Fail immediately and emit a relevant error if column mappings are missing from config.json#595kmoscoe wants to merge 9 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a check to fail immediately with a ValueError if column mappings are missing from the configuration during observations import in DCP_BRIDGE mode, and updates the corresponding tests. The review feedback suggests improving the error message by including the path of the file missing the mappings, and removing redundant columnMappings configurations for MCF files in the tests since MCF files do not have columns.
gmechali
left a comment
gmechali
left a comment
There was a problem hiding this comment.
I ll approve this but we will be refactoring all this shortly for the proper long term behavior here. So no need to do more of these stop gaps. Thanks Kara
|
Oh, that's great. Well, hopefully you can just delete all this code during
the refactor!
…On Fri, Jun 26, 2026 at 7:27 AM Gabriel Mechali ***@***.***> wrote:
***@***.**** approved this pull request.
I ll approve this but we will be refactoring all this shortly for the
proper long term behavior here. So no need to do more of these stop gaps.
Thanks Kara
—
Reply to this email directly, view it on GitHub
<#595?email_source=notifications&email_token=BHMM7UCFXDZE7MZ2Z7DSPG35B2B4BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYGAZDANJYHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4580205886>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BHMM7UBALHQN3GWBTI5EMST5B2B4BAVCNFSNUABFKJSXA33TNF2G64TZHMZTQNZZGQ3TCMRQHNEXG43VMU5TINZUHA4TQNJQGQZ2C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BHMM7UFEWVTGB6FYRTI3G4T5B2B4BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYGAZDANJYHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/BHMM7UEPZTPYNDY4XHP65TT5B2B4BA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJYGAZDANJYHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
2 participants
Again, this is a bit of a stopgap, but the current logic makes no sense, i.e. defaulting the expected columns to variable, entity, date, and value, and then later raising an error saying that "entity" etc. are missing from the column headings. Since this is a major breaking change, it really needs a proper enforcement check and a meaningful error.
Let me know if I should add some tests for this. (It might not be a bad idea to add some DCP config.json tests in general since it has changed so much from CDC.)