Add basic properties implementation #12
Conversation
|
Just realized that the current implementation won't handle multiple exports properly. I'll patch that up and post here when I'm done. |
|
Great to see someone adding new features to the library! |
Unfortunately, this removes introspection for the time being. I plan to add that back in a way that will function much better than it did previously very soon.
colonelpanic8
force-pushed
the
properties
branch
from
f41d674 to
3294dac
Compare
|
The code climate issues are all just XXX warnings, but I want to leave those there for now. I did a major version bump because these changes are backwards incompatible. This became a pretty huge change. Let me know if you have questions. |
|
Here is some code that uses the new interface to export if you would like to test it: |
|
If you are wondering why the changes needed to be backwards incompatible, its kind of a long story... basically I don't think theres a good way around it, and its just going to be better to do things this way moving forward. |
colonelpanic8
force-pushed
the
properties
branch
from
f163bdb to
aff5f85
Compare
|
Do we really need TemplateHaskell, IncoherentInstances? I have some reservations about using these without very serious reasons. |
|
TemplateHaskell is needed for the Lens on PathInfo which simplifies the code significantly. Lens is very widely used in the haskell community. I'm not a fan of incoherentInstances, but it is needed to abstract over automethod with (IO a) since that could refer to the IO () instance or the IO v instance. I would have written that differently but this was the quickest way to get things working without rewriting that class. |
|
Oh, lenses. That's explains, yes. BTW, CodeClimate is mostly for reference, but please make sure CI builds are good. |
|
Tests fixed in #15 changes have become intertwined now :/ |
|
I'd really prefer self-contained changesets. They are much easier to review. |
|
@rblaze Okay, I've done some git surgery on the two branches to make the changes separate again. |
colonelpanic8
force-pushed
the
properties
branch
from
e4d12f5 to
e0ffa7a
Compare
|
hmmm only 7.10.2 failed. Seems to be related to the semigropoids dependency. everything else passed which is strange. |
|
Looks like the semigropoids issue is related to commercialhaskell/stackage#3185 |
lib/DBus/Client.hs
Outdated
|
|
||
| -- NOTE: This instance is needed to make modifyNothingHandler work, but it | ||
| -- shouldn't really be used for much else. | ||
| instance Eq PathInfo where |
There was a problem hiding this comment.
Is it semantically correct that two path instances are only equal if both are null?
Given that PathInfo is exported, it is better to have correct instances, not just working for the narrow case.
There was a problem hiding this comment.
well you can't compare the interfaces because they cointain callable objects, and functions aren't comparable. Otherwise I would have provided a better implementation.
There was a problem hiding this comment.
Let's make a comment stating this clearly, then.
lib/DBus/Client.hs
Outdated
| import Control.Exception (SomeException, throwIO) | ||
| import Control.Monad (forever, forM_, when) | ||
| import Control.Lens hiding (coerce) |
There was a problem hiding this comment.
Nitpick: Module ‘Control.Lens’ does not export ‘coerce’
There was a problem hiding this comment.
It did in one of the older versions of lens. Without this one of the travis builds fails.
There was a problem hiding this comment.
Probably one you removed already. Just checked, all builds have this warning.
Fixes #11, #13, #3