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

Alistair/resource fixes #59

Merged
merged 5 commits into from
Jun 20, 2024
Merged

Alistair/resource fixes #59

merged 5 commits into from
Jun 20, 2024

Conversation

alistairewj
Copy link
Collaborator

Noticed that the snakeCase is not respected in a few files. Not sure how this snuck through as I thought I fixed it all! Added a test to check for it.

@alistairewj
Copy link
Collaborator Author

@ibevers i fixed a few bugs and made sure the tests were passing

having assert NotImplementedError in tests is no fun, hides the real bugs :P

@ibevers
Copy link
Contributor

ibevers commented Jun 14, 2024

@alistairewj Ah, yes, I will avoid having NotImplementedError going forward. What do you think of having warnings instead? That way, we are less likely to forget about tests that need to be implemented, but we can still find the real bugs🐛

@alistairewj
Copy link
Collaborator Author

Well that effectively boils down to putting TODOs in the code, and while I don't have unilateral rules, in my opinion would be better to file those TODOs away in an issue tracker, especially if they're not really attached to existing code.

@ibevers
Copy link
Contributor

ibevers commented Jun 18, 2024

Ah, I see that makes sense

@alistairewj
Copy link
Collaborator Author

Shall I merge?

Copy link
Contributor

@ibevers ibevers left a comment

Choose a reason for hiding this comment

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

I don't understand why precancerousLesions.json is deleted here, but I trust that you have a good reason

@alistairewj
Copy link
Collaborator Author

I don't understand why precancerousLesions.json is deleted here, but I trust that you have a good reason

Yep! I deleted precancerouseLesions.json (note the additional 'e' as a typo). The original file precancerousLesions.json is still there.

@alistairewj alistairewj merged commit 3b7d29a into main Jun 20, 2024
2 checks passed
@alistairewj alistairewj deleted the alistair/resource_fixes branch June 20, 2024 14:58
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.

2 participants