Skip to content
This repository has been archived by the owner on Jul 3, 2019. It is now read-only.

Refactor/sets for membership #18

Closed
wants to merge 3 commits into from

Conversation

Zearin
Copy link
Contributor

@Zearin Zearin commented May 6, 2013

Long time no see! :) This is a small, focused pull request.

Sets are better for membership tests, since they are unordered. These changes simply replace lists or tuples with sets for membership tests (order doesn’t matter).

@Zearin
Copy link
Contributor Author

Zearin commented Jun 11, 2013

Ping! This is a nice, small, focused, pull request. (I’m addressing your concerns from earlier pull requests that there were too many changes for a single request.)

What do you think?

@l0b0
Copy link
Owner

l0b0 commented Jun 11, 2013

I have a very hard time accepting that this will make any appreciable difference on the microscopic lists in question.

@Zearin
Copy link
Contributor Author

Zearin commented Jun 12, 2013

I have a very hard time accepting that this will make any appreciable difference on the microscopic lists in question.

It probably won’t.

But, it definitely won’t hurt anything, either.

@l0b0
Copy link
Owner

l0b0 commented Jun 12, 2013

As far as I can tell it doesn't make the code easier to understand either, and it provides no bug fixes or features.

@l0b0 l0b0 closed this Jun 12, 2013
@Zearin
Copy link
Contributor Author

Zearin commented Jun 12, 2013

Sigh…Fine.

I get it. You’re not interested in Pull Requests. Kind of makes me wonder why you’re on GitHub, though.

Or maybe it’s just me? You are trying to discourage me from bothering with your project?

Well, you have succeeded.

Good luck. I’m done.

@l0b0
Copy link
Owner

l0b0 commented Jun 12, 2013

Sorry if it discourages you Zearin, but you're the only person I've ever refused pull requests from, and it's absolutely not personal. Every commit has to have a purpose, and I fail to see how this one actually improves the code base in any measurable way. Bug fixes, feature additions, even whitespace cleanup I would welcome. If it improves the code, it's welcome. If I don't think it does, you're still of course very welcome to maintain a fork, and sync with this repository (or not) as you wish. It's open source after all. And if others agree that your fork is better, they will start using it. I don't care which fork people use, and it's still completely up in the air whether I'll improve this code myself in any measurable way, so it would be great if someone had the time and energy to work on issues.

Also, I have incorporated several commits from the other pull request which were grounded in best practices. They definitely improved the code base.

@Zearin
Copy link
Contributor Author

Zearin commented Jun 12, 2013

Sorry if it discourages you Zearin, but you're the only person I've ever refused pull requests from, and it's absolutely not personal. Every commit has to have a purpose, and I fail to see how this one actually improves the code base in any measurable way. Bug fixes, feature additions, even whitespace cleanup I would welcome. If it improves the code, it's welcome. If I don't think it does, you're still of course very welcome to maintain a fork, and sync with this repository (or not) as you wish. It's open source after all. And if others agree that your fork is better, they will start using it.

…Understood. Thank you for explaining; that makes thinks clearer for me.

I apologize for my earlier tone. I was frustrated, but I let it get to me, and my words were unprofessional.

deep breath


Alright: Would you be interested if I refactored your custom errors into a separate module?

(I tried this a long time ago—but I think I can make a much better attempt now.)

@l0b0
Copy link
Owner

l0b0 commented Jun 12, 2013

That's cool, I felt a bit of the same after the Git project put a pull request on hold.

Regarding the refactoring, at the risk of committing third-degree hubris, I think the code is not in such a bad shape at the moment. More importantly, several features are missing. Some easy ones:

Harder ones:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants