search

rmagick / rmagick

Ruby bindings for ImageMagick
https://rmagick.github.io/
MIT License
696 stars 140 forks source link

Added delay_pre function and bnw function #294

Closed ghost closed 5 years ago

ghost commented 6 years ago

Hi Sir, I am Adit Mehta, GSoC applicant, interested in contributing RMagick project, first-year student of DAIICT. While I was learning RMagick I was amazed by the ability to create GIF. I got to know that delay feature was adding same time to every picture of GIF but then I thought of adding a new function which will able to give time to every picture specifically according to the user.

vassilevsky commented 5 years ago

Thanks, Adit. Can you also write tests for this function? You can use RSpec if you want. See CONTRIBUTING.md for instructions.

ghost commented 5 years ago

Yes, sir I am new to this I am not that much familiar with RSpec please provide me guidance.

ghost commented 5 years ago

Sir @mockdeep please help me, how I could create tests using RSpec? I am new to this.

vassilevsky commented 5 years ago

You modified file lib/rmagick_internal.rb. The corresponding spec name should be spec/rmagick_internal_spec.rb. Create it and put a test for your method there. You can open other *_spec.rb files and see how it's done. Also, read best practices at http://www.betterspecs.org Good luck!

vassilevsky commented 5 years ago

I've added links to RSpec guides to CONTRIBUTING.md

ghost commented 5 years ago

@vassilevsky Thanks, sir

ghost commented 5 years ago

@vassilevsky Sir, I have added RSpec tests please review it.

vassilevsky commented 5 years ago

Thanks, Adit. I want to see how well your tests cover your functions. For that I have to set up coverage tracking. This repository already sends data to CodeClimate, but not coverage yet. I will add coverage there and then it will be easier to review all pull requests. I will try to do this ASAP.

ghost commented 5 years ago

Okay, Sir

ghost commented 5 years ago

Sir, @vassilevsky are there any changes I need to do in pull request?? Thanking you in anticipation

ghost commented 5 years ago

Sir, are there any issues in commit that i should fix?

ghost commented 5 years ago

Sir, are there any changes that I need to make?

ghost commented 5 years ago

Sir, I made the changes. Please review it.

vassilevsky commented 5 years ago

Github says that this branch has conflicts that must be resolved. So, I can't merge your branch to this project, even if I want. Could you resolve them?

Here is a description of the process:

https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/

mockdeep commented 5 years ago

Hi @aditmehta9, thanks so much for the time you spent on this! Unfortunately, I don't think it's a great fit for RMagick. The delay_pre= method doesn't seem to map to anything in ImageMagick and it looks like something that is for a very specific use case. As for black and white, you can already accomplish this using RMagick:

image.quantize(256, Magick::GRAYColorspace)

This is likely to be much faster, as well, since it is written in C instead of Ruby. Sorry you had to spend so much time on this only to have it rejected. Hopefully it doesn't discourage you from contributing to open source in the future.

aditmehta9 commented 5 years ago

Thank you, sir, for your feedback.