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

make label a required argument (Fixes #131) #426

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

cwmeijer
Copy link
Contributor

@cwmeijer cwmeijer commented Dec 6, 2022

I changed the signatures of all explain_image or explain_text definitions. I changed the label kwarg into a positional argument. No need for error checking because it is now automatically a TypeError to call any of those methods without a labels argument.

@cwmeijer cwmeijer marked this pull request as ready for review December 6, 2022 15:51
@loostrum
Copy link
Member

loostrum commented Dec 6, 2022

The error checking is at least working as advertised, I see a whole bunch of TypeErrors in the tests :D

@cwmeijer cwmeijer marked this pull request as draft December 7, 2022 08:16
@cwmeijer
Copy link
Contributor Author

cwmeijer commented Dec 7, 2022

Always great rushing commit out 1 minute before going home ;-) Back into draft it is.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cwmeijer cwmeijer marked this pull request as ready for review December 9, 2022 09:19
@cwmeijer
Copy link
Contributor Author

Everything should be OK now.
@loostrum could you have another look?

Copy link
Member

@loostrum loostrum 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! I don't understand why the fair-software CI is failing, it seems unrelated to this PR.
The linter is complaining about some dashboard things that mostly seem unrelated as well, but there is one about the labels argument you may want to check out before merging, see https://github.com/dianna-ai/dianna/actions/runs/3655705906/jobs/6177325029#step:4:13

@elboyran
Copy link
Contributor

Signed commits issue. @cwmeijer would you know how to merge this one?
Also linting errors.

@cwmeijer cwmeijer merged commit bbe90f8 into main Jan 17, 2023
@cwmeijer cwmeijer deleted the 131-make-label-a-required-argument branch January 17, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants