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

Allow forcing of bot flag to false #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MusikAnimal
Copy link
Contributor

The default settings for recent changes and watchlists usually hides bot edits, however sometimes bots may want to show up in these feeds but still retain the bot flag for other API operations (so as to not hit throttle limits, etc). Here we check if options[:bot] != false meaning it was explicitly passed in as false, as opposed to nil

@MusikAnimal
Copy link
Contributor Author

Also shouldn't we bump the gem version, at least for my other recent PR that merged in?

@jpatokal
Copy link
Owner

I don't understand this change at all? @options[:bot]will return false for both "false" and "nil", so tagging options[:bot] != false && to the front makes no difference.

Going forward, please submit tests along with any changes you make to demonstrate that they actually do something.

@MusikAnimal
Copy link
Contributor Author

Sorry, allow me to explain. First off @options are those set when you first instantiate MediaWiki::Gateway, correct? So in this use case I have @options[:bot] set to true as I don't want to hit throttles with the API, but I also don't want to edit as a bot. So in that case I'll never be able to pass in options[:bot] as false because it will see that @options[:bot] is true.

With this change it will see options[:bot] is false and short-circuit the if statement as false, never getting to the (@options[:bot] || options[:bot]) part. Otherwise if it's nil (developer didn't pass in a value) or true it will default to normal behaviour. Hopefully that makes sense.

Writing tests is my weakness... but I shall try!

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