[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: So, not to yank your chain, but... I'm okay with checking this in. Yes, we're already in beta, but ModuleSpec is brand new, and the sense I get is that this use case is obscure even for ModuleSpec. The only installed base is beta 1 users, and given that this is new functionality... Anyway. I expect to tag late tonight, say in twelve hours. Can you check it in before then? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: I'll commit it in a little while. Thanks. -- versions: +Python 3.4 -Python 3.5 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Roundup Robot added the comment: New changeset a72a0e4dad20 by Eric Snow in branch 'default': Issue #19927: Add __eq__ to path-based loaders in importlib. http://hg.python.org/cpython/rev/a72a0e4dad20 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Changes by Eric Snow ericsnowcurren...@gmail.com: -- resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: You broke buildbots. Please fix. http://buildbot.python.org/all/builders/AMD64%20FreeBSD%2010.0%203.x/builds/1389 -- assignee: - eric.snow priority: normal - high resolution: fixed - stage: committed/rejected - needs patch status: closed - open ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: Hmm, hard to see how you caused that with the path loader change. Still please take a quick look. I fired off another build to see if it was a transient error, but that'll take a while. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: I'll take a look. It could be something with #19713 or #19708 that also failed there. The other failing buildbot for those 3 changesets is http://buildbot.python.org/all/builders/x86%20Windows7%203.x/builds/7800. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: The windows buildbot failure looks like a race condition in a test unrelated to my changes (see issue #20127). I'm looking at the FreeBSD failure now. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Which passed on the subsequent run... -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: The FreeBSD failure happened in test_threading (apparently), where it was the last test to finish. In the passing run it finished 339/388 -- the seed was different (1253928 vs. 5389019). This does not seem to be related to my 3 changesets. I'm guessing it's one of those lingering race conditions we have sprinkled here and there. -- status: open - pending ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Changes by Eric Snow ericsnowcurren...@gmail.com: -- status: pending - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: I'm fine with this. Thanks, Larry, for your attentiveness and diligence. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: 1. Is this patch going to change best practice for working with ModuleSpec? 2. If we delayed it to 3.5, will users have to ignore it to work around the deficiencies of the ModuleSpec implementation in 3.4? I'm guessing the answer to both of these is well, no, not really simply because comparing ModuleSpec objects is not expected to be a regular operation. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: Yes, I think it will just make the third party idiom for testing that the right module was imported to be to check spec.origin rather than comparing specs directly. It's a nice-to-have, rather than something essential that justifies breaking feature freeze. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: That is, I think the answer to both your questions is actually Yes, but it doesn't really matter due to the obscurity of the use case. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: That's not how this works, Eric. I have to give you permission to add a new feature, which I remind you I have yet to do. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: My bad, Larry. I guess I was reading between the lines too much. :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: That reminds me: I ended up working around this in the runpy tests by only checking the loader type was correct in the module specs. With an improved definition of equality for loaders, the runpy tests could be both simplified *and* made more rigorous at the same time (by simply comparing specs for equality, the same as the other module level attributes). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Yeah, it was while writing tests that I ran into this. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: So can you tell me how this will make users' lives easier? I don't really understand the issues involved. But the only concrete thing I've seen mentioned is making testing easier, and that's not worth breaking feature freeze over. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Right now say you have 2 module specs that are the same. The only difference is that the 2 loaders are not the same instance (they were created separately with the same arguments, ergo equal). The two specs will not compare as equal even though they are equal. I expect users will find it surprising if they compare module.__spec__ to another spec that is basically the same (as described above) and it resolve to not equal. I can see this as particularly vexing for importer writers that are switching over to the new spec-based APIs. In my mind, the benefit of removing that unexpected (and aggravating) behavior outweighs the risk that someone is depending on identity-only comparision for the two loader types that are impacted by this change (which were both just added in 3.3). -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: Yeah, I think we can safely leave this to 3.5. -- priority: high - normal versions: +Python 3.5 -Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: Importer writers are already used to __loader__ being annoying, and comparting specs for equality is unlikely to be a common thing (and easily worked around by comparing spec.origin instead) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Unless there are objections, I'll commit this in the next day or two. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Good point, Nick. Here's a patch that follows your recommendation on both points. -- Added file: http://bugs.python.org/file33072/issue19927-loader-eq.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Changes by Eric Snow ericsnowcurren...@gmail.com: Removed file: http://bugs.python.org/file33038/issue19927-loader-eq.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Larry Hastings added the comment: Brett, could you weigh in please? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Brett Cannon added the comment: I'm fine with the suggestions Nick made. While loaders are not technically immutable (and thus technically probably shouldn't define __hash__), they have not been defined to be mutable and mucked with anyway, so I have no issue if someone breaks the hash of a loader by changing an attribute post-hash. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: (my browser farted the half finished report into existence :P ) The __eq__() implementation of the path-based loaders in importlib is just the stock one that compares object identity. So two that are effectively the same compare unequal. This has a material impact on ModuleSpec. ModuleSpec.__eq__() does a comparision of its various attributes, one of them being the loader. Thus most specs will compare unequal even though they are effectively equal. I recommend that we provide a better implementation for SourceFileLoader and friends. Larry: would such a feature addition be okay? -- nosy: +larry title: Path-based loaders lack a meaningful __eq__() implementation.ModuleSpec.__eq__() is highly sensitive to loader.__eq__() - Path-based loaders lack a meaningful __eq__() implementation. ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Changes by Eric Snow ericsnowcurren...@gmail.com: -- Removed message: http://bugs.python.org/msg205515 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Eric Snow added the comment: Here's a patch. -- keywords: +patch stage: test needed - patch review Added file: http://bugs.python.org/file33038/issue19927-loader-eq.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19927] Path-based loaders lack a meaningful __eq__() implementation.
Nick Coghlan added the comment: There can be some interesting backwards compatibility consequences when adding an __eq__ implementation to a class that was previously using the default ID based __eq__: - it becomes unhashable (unless you also add a suitable __hash__ definition) - subclasses with additional significant state may start comparing equal, even though their additional state is not taken into account by the new __eq__ function. For the latter problem, you can alleviate it by comparing the instance dictionaries rather than specific attributes: class Example: ... def __eq__(self, other): ... return self.__class__ == other.__class__ and self.__dict__ == other.__dict__ ... a = Example() b = Example() a == b True a.foo = 1 a == b False b.foo = 1 a == b True (technically this can still change subclass behaviour if they're messing about with slots, but there *is* such a thing as being *too* paranoid about backwards compatibility) The hashability problem is easy enough to handle just by mixing together the hashes of the attributes of most interest: def __hash__(self): return hash(self.name) ^ hash(self.path) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19927 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
