search

l0b0 / vcard

vCard 3 validator, class and utility functions
https://gitlab.com/victor-engmark/vcard
GNU General Public License v3.0
34 stars 8 forks source link

Refactor/sets for membership #18

Closed Zearin closed 11 years ago

Zearin commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

Zearin commented 11 years ago

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 commented 11 years ago

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

Zearin commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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: