Get fans and fan ofs

[omgyeti]

Added ability to get fans and fan ofs from a user using a users id and changed readme to reflect those changes.

[technobly]

Have you tested this? Also the documentation should probably explain that if you don't specify a userid that it will return the data for the bot's userid.

[gizmotronic]

Yes, this change works as indicated. @DubbyTT, were you not able to get it to work?

Also, it's awfully hard to ding someone on the documentation when there are already other instances of exactly this same thing (remDj() and getProfile() readily spring to mind). Maybe someone can contribute general improvements to the documentation instead of holding up this pull request.

[technobly]

I'm not dinging him, I'm just wishing the documentation was complete. He's attempted to document it, but overwrote the existing text kind of incorrectly. It would just take 3 seconds to update his branch. Might as well do it now while we are talking about it. I didn't test it, just looking to see if it has been.

Instead of "Get the list of who the specified user has become a fan of." it should probably be "Get the list of who the specified userid has become a fan of, or who you've become a fan of if no userid is specified"

and "Returns an array of everyone who is a fan of the specified userid, or an array of everyone who is a fan of yours if no userid is specifed"

[gizmotronic]

The proposed documentation change is consistent with the existing documentation. There is no return value; the (non-optional) calback is provided an object, as has been previously documented in turntable_data/getfans.js and turntable_data/get_fan_of.js.

[technobly]

So you are saying because the documentation is not proper right now, we should just make it match and move on? You are really arguing that point?

I have a better idea... how about we care a little bit more to make it right and set an example for someone else to update the rest of the documentation someday in the future?

[gizmotronic]

No; at minimum, I'm saying merge the request, then copy and paste your part about the userid parameter and commit it after the fact. Better would be for someone to scrub the documentation and fix it, instead of doing piecemeal commits to fix them one at a time.

[technobly]

I don't wanna do that... I want @agdurrette to do it :) It's not hard, and sets a good example for future users to add proper documentation.

likeaboss

[gizmotronic]

It works fine, both with and without a userid. My recommendation is that the change be merged.

[Izzmo]

Alright, I'm going to take the lead on this, as I agree with @DubbyTT.

1. It is not our job to test it, but the person making the change request. Thought I would clear that up, as pull requests are not meant for me to copy your changes over to a test environment and make sure they work, but that you prove to us that they work and they will not mess things up when merged and committed.
2. Make the change to the documentation on the userid being specified or not and create a new pull request so that change is reflected. It's easier if this is just all one, and it's not like it's hard to do :smile:

Wish I could interpret damn CoffeeScript better.. ugh. But yeah looks good to me from a high-level overview.

[technobly]

Yeah the code looks good to me... it's basically a copy of getProfile.

[omgyeti]

@gizmotronic he he, I was using it wrong :D

@Izzmo Actually, In my honest opinion, it is everyone job to test it to make sure it works as it should before it gets pushed out and messes things up for all.

[gizmotronic]

For the record, and to be crystal clear: I'm not down with the "documentation quibble holds a pull request hostage" thing, especially given prior art (as in this specific case). Pull requests can be modified before they are merged. This is a perfect case for it, if you are of the opinion that the documentation must be improved prior to merge.

[technobly]

Lots and lots of talk... is it done yet? :)

Good grief.

[omgyeti]

@gizmotronic Glad you mentioned that, I thought I could modify the pull request but was a bit unsure if you could or not when @Izzmo just closed it like he did .-.

@DubbyTT XD I'm working on it, doing a bit more testing with it before a new pull request.

[Izzmo]

@agdurrette I'll bite. How is one supposed to do that without making it very time consuming?

[gizmotronic]

github: Adding commits to existing pull request

[technobly]

Pretty sure you can just reopen this, push one more change of the doc file, and then it can be merged with the three commits.

Obviously it would be nice to just have one tidy commit for all changes, but it's not too big of a deal unless you have like 15 commits due to typos, then it's just sloppy. You can squash commits using the rebase command, but it's a pain in the ass... trust me it would be easier just to delete the forked repo, re-fork it and make your changes again verses doing that.

[omgyeti]

@Izzmo Copy and past is time consuming? Also, I'm not saying that I should not test, quite contrary. I think everyone that can should test to ensure that everything works as it should before it gets pulled.

[technobly]

Re: testing... http://www.youtube.com/watch?feature=player_detailpage&v=Nh7UgAprdpM#t=50s

[Izzmo]

@gizmotronic & @agdurrette I don't think you understand what I am saying. You are laying all the work on someone like me, who really doesn't care about this change, only that it works. I don't want to test it, that's on you. From my perspective, unless a repo manager wants to test it, it will just sit here until you've proven it's tested and ready to go.

Keeping that in mind, what would be acceptable is creating a branch for people to test from, for, example, a beta release, and if people would like to be on the edge of development, they can use the beta version. I feel like this would be more suitable that committing to the trunk.

Just my 2 cents.

[gizmotronic]

What does "repo manager" mean?

[Izzmo]

I'm not sure on the list, but I think it's just @DubbyTT and I.

[gizmotronic]

What list are you referring to? If you mean "has commit access", then no, certainly not.

[Izzmo]

Yes, that's what I mean. Where is that list?

[gizmotronic]

Only the repository owner can see it, under Settings > Collaborators. But, I'm sure that it's not just you two, because I also have commit access. I don't see that this implies any burden whatsoever, other than to err on the side of caution and ask @alaingilbert to be the final word on anything that may be controversial (until he says otherwise, at least).

[Izzmo]

Well, the great thing about any open source project like this is oversight, and that's all that @DubbyTT and I are trying to show you guys.

[gizmotronic]

LOL. I've been working on collaborative open source projects for more than 20 years, and I know all about oversight. It's awfully easy to cross the line from being a good steward to unnecessarily and aggressively questioning minute detail.

[technobly]

Since you obviously want my 2 cents... you come off as pretty abrasive @gizmotronic and not a team player.

[gizmotronic]

Team players would try to engage in dialog before closing pull requests outright; and yes, that probably has colored our interactions since then. Frankly, I thought it was rude and condescending, and awfully presumptive in someone else's repository that you happen to have commit access to.

[Izzmo]

@gizmotronic it was a large chain of comments and was getting no where. I don't really see why this is such a big deal to you, as we are trying to collaborate, but you insist on dwelling on meaningless dialog like what we are doing right now. It gets us off topic, and frankly, you are the only one who cares about us "being mean," or which ever way you would like to describe it.

I stand by my judgement on testing; I agree with you that the community should help test, but it is up to the person making the change to do an initial test to make sure it works from a high level, then commit it to a branch for beta testing and let other people test it before it goes into the trunk. This is pretty typical and not sure why you would fight that logic.

Furthermore, Alain gave access to people like us to make sure things are progressing with this project. I did not know you had access, but judging on your cavalier attitude towards merging changes, I think you should take a step back and make sure you are in the right. I'm nothing if not flexible in being persuaded to new processes, but you are not giving anyone, including myself, a reason to trust you with such access if you believe in this way of merging requests without thoroughly checking them first and putting it through the rounds, so to speak.

Finally, we are being team players. The simple fact that we are even talking about this means we are team players and are willing to talk about something before it's changed. But since @DubbyTT and I do not agree with you, you are getting all up in arms about it instead of being civil and asking us why and progressing the dialog to something useful.

I'm not trying to be the "all mighty and powerful," I am just trying to give my opinion on the matter, since I have not merged a single pull request, but I could shed some insight on things like this as well. Take it how you want!

[technobly]

• deletes comments

you beat me to it...

[omgyeti]

Cat, Im a kitty cat, and I dance dance dance! but srsly.

@Izzmo I'm not saying that you should do all the testing and only you. What I'm saying is that I do testing, you do testing, we all do testing and make sure its running damn smooth before it gets pushed out. Personally, I don't like the It works, Ship it method we are not Microsoft. Me saying that it all works should not be fine, and you should take my word that it works fine. If you want to merge something before testing it, so be it, I'm not stopping you.

Me saying that it all works should not be fine, and you should take my word that it works fine.

Also, I did not fork ttapi so I could go off and work on my own api :P a beta branch would be nice, but is seemingly pointless when each of us have our own fork for testing.

TL;DR Whats wrong with other testing something before an error gets merged.

EDIT: O_0 im not fighting that logic, I agree with it 0_o

[gizmotronic]

I'll break it down: @agdurrette implemented the change, tested the change, and submitted a pull request. I validated that the change worked as advertised. People who do not have commit access, like @agdurrette, can only submit pull requests from their own repository to an existing branch in this one, so let's dispense with that straw man, shall we? He did something that's entirely reasonable and proper on Github.

Now, the only actual difference of opinion was how @agdurrette had documented the change. Nevertheless, the pull request was unnecessarily closed by someone who admittedly isn't really familiar with CoffeeScript, who apparently isn't aware of how Github works, and who doesn't have the time to test simple changes (which, you'll note, is something that @alaingilbert does before merging). You can call that abrasive if you want, but I'm not going to apologize for assuming that other people actually know what they're doing until they prove themselves otherwise.

[Izzmo]

You both are not understanding what I am saying, and possibly @DubbyTT. What @agdurrette is doing is completely fine. We are not talking about him opening a pull request. We are talking about @gizmotronic. But I digress.

Anyways, all I'm saying is that we should handles these appropriately and test them in a beta branch before merging them to the trunk. That's it. If everyone agrees, then so be it, otherwise, I guess I'm outvoted.

[gizmotronic]

Funny place you should choose to comment on me and my alleged cavalier attitude. And, even if this were an appropriate context, the facts are not, on the whole, on your side. I would be happy to detail exactly how @DubbyTT's change was ill-tested and doesn't live up to his/her standards for other people, but that would be mean, abrasive, and otherwise unfriendly. Or so I gather.

[Izzmo]

Man, just let it go.

[technobly]

@gizmotronic The fact is, I'm not perfect... yet. Tell me what you want me to work on and I'll happily consider it. For the record, I did not even update the documentation for my change, but I did provide a nice working example. If I had submitted documentation with errors, I would hope someone would have been nice enough to review it and ask me to correct it. If someone had asked me to update the documentation in addition to my changes, I most certainly would not have complained about it. Honestly it slipped my mind completely, and I have every intention on adding it later after we get through the current changes you are proposing. I tested my code very thoroughly, and two other people chimed in to say all was well. I didn't hear you say otherwise. Your update that clears the ws.onerror handler seems like good practice, but so far I haven't seen the need for it and I certainly don't think that makes my code ill-tested. I also do not have 3rd world internet¹ so a transparent proxy issue is not something I even know about, nor would I have encountered it or thought to simulate it.

If you are ready to get back to being a good steward for TTAPI, I would be very thankful.

¹ My definition for internet that is sub par when compared to standard internet access in the US.

[gizmotronic]

So, in the spirit of collaboration, a tip: both of the problems that I fixed after the fact were things that I discovered in my own similar changes by watching debug output and tracing the events that were fired (which ones, when, how often, etc.) while going through multiple disconnect/reconnect cycles. Since you bring up the error handler, specifically, just as the close event should not be fired when we're about to register in a new room, neither should an error. I observed an error when reconnecting that occurred when roomRegister() attempted to close a websocket that was no longer connected. That's meaningless in roomRegister() as we're about to destroy the existing websocket object and create a new one, anyway. This change specifically eliminated the possibility that a naive event handler (in the user's bot) will schedule an unnecessary reconnection attempt because of the spurious error.

Unfortunately, you end up casting aspersions about once again. Is it really necessary? There are valid reasons that someone might be behind a filtering proxy, and it's rarely something under an individual's direct control. You don't have to like this to accept that it's at least reasonable. I'd really appreciate you assuming that other people are not just blowing smoke, because up to this point, I haven't felt that at all.

[technobly]

You have completely lost me... sorry, I don't understand anything you just said. Please include less run-on's, big words I need to look up, sarcasm, negative logic and idioms. Seriously, just talk to me normally like we are still friends and maybe we can get past this...

[gizmotronic]

Not a single word of that was sarcasm. I'm writing to you conversationally, not formally, so yes - things to tend to be run-on sentences. I'm trying to speak to you like a friend would speak to you because I want this resolved.

I try to re-state the last paragraph. You have minimized and dismissed others' ideas. Your tone has been judgmental and bossy. The only apparent reason is that you either personally dislike the ideas or don't understand why they may be useful. You also criticize style and vocabulary. I feel that all of these things are hurtful and unproductive.

[technobly]

Oh I thought you were going to explain something factual about the TTAPI.

If you scroll back in time, there was a point at which I would gladly merge your changes after testing them... however my opinion is that you are so focused on finding errors in everyone else that you are the cause of all this un-productivity.

If there was a way I could ignore your comments, I would.

@alaingilbert I would appreciate it if you could slap us all on the back of the hand please, because frankly I'm really tired of this back and fourth bickering.

[alaingilbert]

@Izzmo I completely agree with the beta branch. I'm going to create it.