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

Improve neuropixels calibration file error messages #230

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Aug 20, 2024

- Remove dangling ")" from NP2.0 missing calibration file message
- Add SN information to NP1.0 exception messages
@bparks13 bparks13 added this to the 0.2.0 milestone Aug 20, 2024
Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

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

These are good changes, however I mistakenly updated these same error messages in #231 while also changing/adding how we parse the file. Currently there will be merge conflicts between the two of them; I suggest we either pick and choose what to keep from #231 and port it here to keep the two PRs completely separate, or we merge these changes with what I did there and close both issues from that PR.

@jonnew
Copy link
Member Author

jonnew commented Aug 21, 2024

I think merge conflict is fine, we just need to resolve. The conflict will be in the XML tag only, right? If so it's actually beneficial because we can take the best text from each. This will be merge commit but manually moving them would be another commit as well.

@bparks13
Copy link
Member

@jonnew There will be merge conflicts outside of the XML tags, I modified the methods so that all parsing for a single calibration file was done inside of a single method, thus allowing me to reuse the same functionality in the GUI. However, we have since decided to remove this file parsing in the GUI until we can check against a schema. I would like to keep those parsing methods intact, as it will make it easier to integrate into the GUI later on, but that conflicts with these changes. It shouldn't be a huge conflict, so I guess it's not a big problem, but I know merge conflicts can be annoying.

Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

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

Approving these changes, we can resolve any conflicts as they come up.

@jonnew
Copy link
Member Author

jonnew commented Aug 22, 2024

@bparks13 I guess I would like to review the changes to the parsing code before merging. Your changes are in the commit history so they can be recalled if needed, espeically if you create and issue with a link the the commit.

@jonnew jonnew merged commit 79e3842 into main Aug 22, 2024
6 checks passed
@jonnew jonnew deleted the issue-138 branch August 22, 2024 15:27
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.

ConfigureNeuropixelsV1e node error messages improvements
2 participants