Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add resetOption method #375

Merged
merged 1 commit into from
Jan 20, 2020
Merged

add resetOption method #375

merged 1 commit into from
Jan 20, 2020

Conversation

qroques
Copy link
Contributor

@qroques qroques commented Oct 25, 2019

No description provided.

@qroques qroques requested a review from alexpozzi October 25, 2019 11:58
@qroques qroques self-assigned this Oct 25, 2019
@alexpozzi
Copy link
Member

Thank you for your contribution!

Can you please fix StyleCI errors, add a test case for this new method and add a little documentation on how to use it in the README.md file?

README.md Outdated
@@ -80,6 +80,17 @@ $snappy->setOption('toc', true);
$snappy->setOption('cache-dir', '/path/to/cache/dir');
```

### Reset options
Options can be reset to their initial values with `resetOption()` method.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Options can be reset to their initial values with `resetOption()` method.
Options can be reset to their initial values with `resetOptions()` method.

$media->setOption('disable-javascript', true);
$media->setOption('no-background', true);

$media->resetOptions();
Copy link
Member

Choose a reason for hiding this comment

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

Can you assert that options has been set before calling resetOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting option is already tested with testSetOption() method. I don't know we should test it here.

@alexpozzi alexpozzi mentioned this pull request Nov 8, 2019
@qroques qroques requested a review from alexpozzi November 13, 2019 14:02
@stale
Copy link

stale bot commented Jan 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 12, 2020
@alexpozzi alexpozzi removed the stale label Jan 13, 2020
@alexpozzi alexpozzi force-pushed the feature/resetOptions branch from 14869c4 to ac05bbc Compare January 20, 2020 08:11
@alexpozzi alexpozzi merged commit 7bac60f into master Jan 20, 2020
@alexpozzi alexpozzi deleted the feature/resetOptions branch January 20, 2020 08:30
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants