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

Added code to raise exception with string #1952

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

stef405
Copy link
Contributor

@stef405 stef405 commented Apr 17, 2023

I added two lines to VideoClip.py that raises an exception if a user passes in a string as the color parameter. This is in reaction to this Bug. The problem the original user faced was that it raised a ValueError. The user inputted 'black' as the color in ColorClip, which is not an acceptable parameter for the function. Instead, I just created some code that checks if color is a string, and raises an exception that mentions the wrong user input. In the future, a user should hopefully realize quicker that they passed in incorrect input with the Exception message, instead of not knowing why a ValueError was thrown later on.

To test_color() in test_VideoClip.py, I added a test that attempts to create a Clip with color = 'black'. The test passes, since an exception should be thrown when color is passed in as a string.

I also checked for PEP8 conventions in both files, and none of the code I added resulted in a style complaint.

  • I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
  • I have added suitable tests demonstrating a fixed bug or new/changed feature to the test suite in tests/
  • I have properly documented new or changed features in the documentation or in the docstrings
  • I have properly explained unusual or unexpected code in the comments around it

@keikoro
Copy link
Collaborator

keikoro commented Apr 19, 2023

@stef405 I fixed the code for the checkboxes and moved it to the end of your PR description. They are meant to be ticked off to give an idea of what was done at a quick glance.

@keikoro
Copy link
Collaborator

keikoro commented Apr 19, 2023

Looks like the formatting needs some more work.

@coveralls
Copy link

Coverage Status

Coverage: 83.917% (-0.8%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage: 84.048% (-0.7%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage: 84.048% (-0.7%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage: 84.513% (-0.2%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

@coveralls
Copy link

coveralls commented Apr 19, 2023

Coverage Status

Coverage: 84.828% (+0.08%) from 84.751% when pulling 3aa1a4d on stef405:master into 86fd511 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage: 84.73% (-0.02%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage: 84.73% (-0.02%) from 84.751% when pulling fe13e43 on stef405:master into 86fd511 on Zulko:master.

@keikoro
Copy link
Collaborator

keikoro commented Apr 20, 2023

@stef405 You can combine the changes into one commit and force-push the PR, no need to add to the log when it's something so small which could have been caught earlier.

@stef405
Copy link
Contributor Author

stef405 commented Apr 20, 2023

@keikoro I combined the commits. There should now be only one commit associated with this PR

@mgaitan mgaitan merged commit b5bb086 into Zulko:master May 26, 2023
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.

4 participants