-
Notifications
You must be signed in to change notification settings - Fork 613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General restructurisation of repository #328
Comments
👍 |
-1 Brings no advantage, complexifies the code, adds visibility issues. |
@tomaka Libraries that rely on one or two image formats can depend on those directly. Generic libraries that don't use formats can depend directly on the trait library. |
Just use Cargo features, they exist specifically for this situation. |
@tomaka Cargo features aren't good for long term maintenance of a library. |
But I really thing we have bigger problems than the structure of our lib. First we need to maintain and improve the decoders. PNG and JPEG are both not feature complete and bug free. GIF is ok I hope and the rest… |
As a default, I think a good structure means more crates, because it allows it the project to grow. Once you have crates, it is easier to move it to another repo. You also get more choices, whether you want to add features that are specific to some format, or you can add new features step-wise without every format needed to support it. More crates also means they can be built in parallel, and the generic abstraction can be used separately. |
As @bvssvni said. This will help with #311 and also it will make adding new formats easier. @tomaka IMHO it will improve code quality and reduce complexity (exept initial separation). Also this IS usage of cargo features. To be precise Path Dependencies. @nwin I totally agree. But dividing it into pieces will help with improving quality of decoders. Also writing tests will be easier, as we can separate them by repo and run only for given format (faster, easier and without crap from others formats). |
Are path dependencies compatible with crates.io? |
I'm with @tomaka on this one. Cargo features make way more sense. |
@hauleth We're talking about http://doc.crates.io/manifest.html#the-[features]-section I don't see why they wouldn't be good for long term maintenance. |
@hauleth We aren't talking about "features of Cargo", we are talking about "the |
@tomaka Features are very usage dependent, you turn them on/off, but it is not a proper abstraction. It is useful when you have different platform targets and special code that works for different platforms, but when we're talking image formats, then you want to control dependencies. Features are a way of tangling up stuff, putting more states to reason about a library. It is not the same semantics as "I want to depend on X" and in what way you want to depend on it. You can't reexport stuff properly. It is like having a multi-dimensional dependency, it behaves a bit weird and might interact with other dependencies in some unexpected way. When using those long term, you have to reason "am I using image with TIFF?" and when you want to extend the library, you have to think about what features are turn on/off and how this affects the dependency graph. As a project grows in complexity, this becomes harder to reason about. I was picturing features as a reasonable way to solve the current problems, but when thinking long term, I can easily see how this can mess stuff up. |
Usually we let things rest for some days or weeks to avoid bad decisions, but in this case I thought this was an obvious improvement to the library and immediately approved on the idea. However, it might turn out to be more complicated. I doubt anyone has a clear picture of how this will affect the project yet, so I recommend the following:
@nwin mentioned stabilization that could be important. I'd like to get into this in more detail, but in another discussion because it touches a larger context. |
An analysis of Cargo features vs modular crates is needed, so I opened PistonDevelopers/piston#852 |
No, it is the exact contrary. Features must not be used for platform-specific code, or annoying compilation errors happen.
Features are designed to work well with dependencies. Features are not on/off flags, they can only be turned on and not turned off.
The dependency graph never changes when you enable or disable a feature.
How is this different if you split image into multiple crates ? |
Abstraction-wise it does not make any difference whether we use features or crates. Ideally image(feature=png) would only contain an image en-/decoder. That would have the same effect as having an extra image-decoder-png crate. The advantages of crates I see:
But generally any restructuring that would allow us to use crates would also immediately allow us to use feature flags. This is really not a blocker at all. If we are able to use feature flags switching to crates is easy. It is merely some copy & paste. @bvssvni Just du make a clear point: Using feature flags is not a bad decision that should be avoided. In contrary, it is be a step towards splitting everything up into crates. I see using feature flags as a no-brainer which we should do. Splitting everything into crates is something I’m happy to discuss but we should postpone this decision. |
Just a moment. But why cannot we use both of them? Features only enable inclusion additional crates that are spltitted into tree. Is there any problem with that? [features]
jpeg = ["jpeg"]
[dependencies.jpeg]
path = "formats/jpeg" and it should work as charm. |
Because the point is not whether we can combine feature flags with crates but whether we should split the stuff into crates at all. As @tomaka already said in his first post, crates influence how we have to handle visibility. We could not have any internal non-public API any more and I’m not sure we’re at that point yet. |
It would be great if we split all formats into separate libraries (without additional repo). Original repo should work only as a glue between all packages.
Also I'm working on extracting
math::nq
to separate repo.Todo
formats/*
.
Zero
/One
/Primitive
traits and replace them withnum
equivalentsimage
as a metapackage with some glue between newly created componentsThe text was updated successfully, but these errors were encountered: