Clean up API and documentation for the 1.0.0 release

[s-ludwig]

This mostly just adds documentation comments. Some code got deprecated/removed and some code got moved. Review should be pretty straight forward, despite the size of the diff.

[s-ludwig]

Okay, the API should be good to go for 1.0.0 now (apart from a change in PackageRecipe.getPlatformSettings). A lot could still be done to modernize certain aspects (using ranges mostly), but IMO it doesn't make much sense to make that a 1.0.0 priority at this point. I'll rebase and squash related commits, so that reviewing commit-by-commit makes sense. Apart from the first commit, there are no functional changes, so concentrating mostly on the API/doc aspects should be sufficient for the review.

[s-ludwig]

Rebase finished. Review status page: https://github.com/D-Programming-Language/dub/wiki/API-review-for-1.0.0

[JackStouffer]

There's something to be said for Walter's policy of breaking up PR's by commits :/

[JackStouffer]

import std.array : appender;

[s-ludwig]

There's something to be said for Walter's policy of breaking up PR's by commits

What difference would that make in this case, though, apart from greatly increasing the overhead for PR creation/merging? I've tried to make one commit for each step, so that reviewing commit-by-commit reduces the pain a bit.

[wilzbach]

maybe add a removal date?

[s-ludwig]

My plan is to remove all deprecated symbols for the 1.0.0 release, so that we can start with a clean slate. So instead of a date, I'd mention the version, but I'll add that for all deprecations throughout the API.

[wilzbach]

I found a bit of time and had a look over this.
@s-ludwig yep this really just moves around a lot of stuff and adds documentation.
Imho this is sitting here far too long - let's go with!
We won't spot bugs without an alpha release anyhow ;-)

Maybe you want to set a removal date for the deprecation and annotate them so it's easier to find?
(Btw SemVer allows breaking changes for major releases)

[s-ludwig]

Okay, deprecations are augmented with removal notes now and everything is rebased onto master.

[PetarKirov]

Hi Sönke,

Here's my review:

General suggestions:

• Group dub.semver, dub.dependency and dub.dependencyresolver into a
  package for dependency resolution.
• Group dub.description, dub.init, dub.package_, dub.packagemanager,
  dub.packagesupplier, dub.project and dub.recipe.packagerecipe into a package for DUB package management.
• Group dub.recipe.io, dub.recipe.sdl, dub.recipe.json and dub.internal.utils and some parts for dub.commandline
  into a package for IO related functionality
• Leave only dub.dub, dub.commandline and dub.version_ as top level dub.* modules
• Use selective imports everywhere
• Use local imports if something is used in < 5-10% (or in only 2-3 places) of the whole module
• Avoid global variables
• Don't use exceptions for return values (e.g. determineVersionFromSCM
  should return either a valid Version or Version.init (== Version.unknown)).
• Don't catch bare Exceptions
• Remove constructor overloads that do IO
• 80% of the code should be pure and separate from the code that does the IO
• Use ranges instead of arrays and strings where possible.
  Prefer lazily algorithms. Avoid GC usage. Remove things like
  .map!(f => cast(string)f.filename).array;. Change iterator delegates to ranges.
• Avoid trivial @propertys - instead make the particular fields public and const (where applicable). Use properties only when more compicated logic is needed than plain get/set.
• [very low priority]: Use phobos code formatting - braces, indentation, etc. - (although I personally prefer snake_case for local variables).
• I see no point in deprecations as DUB is still not 1.0.0, and if I were you, I would just remove the old stuff, but I leave this decision to you.

Commit-by-commit review:

• bfe27fb (Remove pointless extra abstraction for package recipes)
  • the changes look good
• a11a280 (Remove PathAndFormat struct)
  • all good
• 0839474 (Code cleanup)
  • the change is OK, but:
  • constructors that do IO are a bad idea. Instead remove this overload and add a unittest showing the user how he can accomplish the same by manually reading the file.
• 357f5f2 (Clean up and document the dub.package_ public API)
  • the change is OK, but:
  • Maybe Package.dependencies should be named Package.allDependencies
  • Change Package.allDependencies documentation to: "Returns a union of the dependencies of all configurations" (it sounds more clear this way).
  • Explain which version will Package.allDependencies when there is conflict - the newest, the oldest, or a union?
  • Merge Package.getDependencies into Package.allDependencies
• b58ad63 (Review dub.semver and improve documentation)
  • the change is OK, but:
  • it seams there is some overlap with dub.dependency. Move more stuff from there to this module?
• e48ec49 (Review dub.init and improve documentation)
  • all good
  • Maybe initPackage should support custom user-defined init templates in the
    future (other than minimal, vibe.d and deimos).
• a0eb519 (Review dub.description and improve documentation)
  • all good
• 53d838f (Move BuildPlatform to dub.platform and improve docs)
  • the change is OK, but:
  • I'm not sure determineBuildPlatform and friends are useful. They just provide information about the DUB binary. They can only be useful when the package target platform is the same as the platform on which DUB was compiled.
  • There is duplication with dub.compilers.utils.generatePlatformProbeFile. Refactor with mixins?
• 5a6cd75 (Review and improve documentation for dub.commandline)
  • all good
• 77c4d18 (Review dub.recipe.io and improve documentation)
  • Rename readPackageRecipe(Path file, ...) to Path filename so that it shows up in documentation next to string filename.
  • otherwise all good
• df8fb07 (Make internal function in dub.recipe.json private.)
  • all good
• 59bb9aa (Review dub.dependency and improve documentation)
  • the changes look good, but:
  • Dependency should be backed by an Algebraic (or something conceptually similar), because it can't be both a specific version, version range and path at the same time.
  • Static properties like any, invalid, etc should have Dependency return type for clarity, instead of relying on return type inference
  • Magic constants like ANY_IDENT, "~master", etc should be all enums declared next to each other in the code
  • No need for trivial get and set properties - expose fields directly. Use properties only when more compicated logic is needed.
  • Add more comments on the internals of Dependency.versionSpec
  • There are Dependency.fromJson and Dependency.toJson. What about SDL?
  • There is some overlap between Version and Dependency. I think you simplify the code if you separate it more cleanly.
• 6d616ee (Fix naming convention for the compiler handler classes and add documentation)
  • the changes look good, but:
  • dub.compilers.compiler and dub.compilers.[dmd|gdc|ldc] import each other.
    Break the import cycle by moving all but the dub.compilers.compiler.Compiler
    interface to dub.compilers.package. Maybe rename dub.compilers.compiler
    to dub.compilers.interfaces
  • Now I saw you did some of that in 5b2f303, but there are still a couple of things left
• 706d03b (Review dub.generators.generator and improve documentation)
  • all good
• dd1a065 (Make all internal generator functionality private)
  • all good
• bec5507 (Avoid any of the newly deprecated symbols)
  • all good
• 95da174 (Review dub.packagemanager and improve documentation)
  • the changes look good
• 5b2f303 (Move utility functions from dub.compilers.compiler to dub.compilers.utils)
  • the changes look good, but:
  • there are still some misc stuff left in dub.compilers.compiler that shouldn't be there
  • Move the isLinkerFile unittest from dub.generators.build to dub.compilers.utils
• 85d1540 (Add documentation for dub.compilers.compiler/utils)
  • all good
• 63f8612 (Review and document dub.packagesupplier)
  • all good
• 2e8fa59 (Review and document module dub.dub)
  • the changes look good
• 088633e (Review and document dub.project)
  • the changes look good
• a49678f (Make defaultPlacementLocation a member of Dub instead of a global)
  • the changes look good
• 62122b8 (Fix behavior of "--data-list --data-0" and improve the Dub/Project.listBuildSettings API)
  • the changes look good
• 660c2b5 (Review fixes)
  • all good
• bc3c1c5 (Mention version of removal for all deprecated public symbols)
  • all good

Overall this PR LGTM.
Most of my complaints are about better organization of the code, making most of the functions pure (by moving out the IO), local imports, less arrays and more ranges, and DRY.

[PetarKirov]

@JackStouffer I personally found it easy to do a commit-by-commit review. Most commits add documentation, move stuff around, or change names / add deprecations. But you still need look at each module as a whole to get a sense of the bigger picture.

[s-ludwig]

@ZombineDev: Thanks a lot for the detailed review! I agree with almost all of the points, but I think that the time frame isn't sufficient to implement most of them. The goal is to get DUB to 1.0.0 ASAP and just avoid the most prominent kinks in the public API. That's why I restrained my changes to the most obvious and least intrusive ones for now.

But 2.0.0 should definitely aim for a thorough design revision - the codebase is dated, D and Phobos changed a lot over the years, and the first months of development were rather rushed, to get something that works fast (as a package manager only for vibe.d and not intended as a general purpose tool). When this PR is merged, I'll put your remaining points into an issue or on the wiki so that they can eventually get processed after the 1.0.0 release (can happen additively, flagging the old APIs as "scheduled for deprecation").

As the first step, I'll go through the specific points for each commit (might take some time, but hopefully I can allocate a good chunk of time tomorrow).

BTW, regarding deprecations, I don't think that pre- or post-1.0.0 really makes a practical difference. Some projects already use DUB as a library and not having a grace period for breaking changes is always a pain point (requiring static if workarounds or adjusting foreign code bases), so I like to do that anyway.

[coveralls]

Coverage decreased (-0.5%) to 60.252% when pulling 484d88d on api-cleanup into 494acb7 on master.

[s-ludwig]

• 0839474 (Code cleanup)
  • the change is OK, but:
  • constructors that do IO are a bad idea. Instead remove this overload and add a unittest showing the user how he can accomplish the same by manually reading the file.

Replaced the constructor by a static load() method.

• 357f5f2 (Clean up and document the dub.package_ public API)
  • the change is OK, but:
  • Maybe Package.dependencies should be named Package.allDependencies
  • Change Package.allDependencies documentation to: "Returns a union of the dependencies of all configurations" (it sounds more clear this way).
  • Explain which version will Package.allDependencies when there is conflict - the newest, the oldest, or a union?
  • Merge Package.getDependencies into Package.allDependencies

Replaced .dependencies by .getAllDependencies(), which returns a list instead of an AA.

• b58ad63 (Review dub.semver and improve documentation)
  • the change is OK, but:
  • it seams there is some overlap with dub.dependency. Move more stuff from there to this module?

You mean the Version class, right? The only issue is that Version also supports branch identifiers and maybe commit references in the future, so it wouldn't fully fit in a semver module.

• e48ec49 (Review dub.init and improve documentation)
  • all good
  • Maybe initPackage should support custom user-defined init templates in the
    future (other than minimal, vibe.d and deimos).

Definitely on the radar.

• 53d838f (Move BuildPlatform to dub.platform and improve docs)
  • the change is OK, but:
  • I'm not sure determineBuildPlatform and friends are useful. They just provide information about the DUB binary. They can only be useful when the package target platform is the same as the platform on which DUB was compiled.

They are useful when DUB is used as a library. I'm for example using it for a CI slave, so that it can report the actual platform for a particular set of compiler switches.

• There is duplication with dub.compilers.utils.generatePlatformProbeFile. Refactor with mixins?

Should definitely be done. I'll get to this later, since it's not part of the API.

• 77c4d18 (Review dub.recipe.io and improve documentation)
  • Rename readPackageRecipe(Path file, ...) to Path filename so that it shows up in documentation next to string filename.

Done.

• 59bb9aa (Review dub.dependency and improve documentation)
  • the changes look good, but:
  • Dependency should be backed by an Algebraic (or something conceptually similar), because it can't be both a specific version, version range and path at the same time.

Good point. I think this needs to be postponed to after 1.0.0, though.

• Static properties like any, invalid, etc should have Dependency return type for clarity, instead of relying on return type inference

Done.

• Magic constants like ANY_IDENT, "~master", etc should be all enums declared next to each other in the code

Do you mean globally in the module? They are now next to each other within their struct context. Naming convention still needs to be fixed, but they are not public.

• No need for trivial get and set properties - expose fields directly. Use properties only when more compicated logic is needed.

At least the optional/default ones could still come in handy, because it would make sense to switch over to using an enum (dub.dependencyresolver.DependencyType) to avoid the redundant state optional == false && default == true.

• Add more comments on the internals of Dependency.versionSpec

Do you have something specific in mind? Maybe the behavior of the "fuzzy" version match could be explained there, or a link to the relevant section in the package format specification could be added.

• There are Dependency.fromJson and Dependency.toJson. What about SDL?

The SDLang serialization code is in dub.recipe.sdl, but is private. The toJson/fromJson methods are there for generic serialization and because they are used in other places (dub.selections.json), but I'd really rather like to remove them, too.

• There is some overlap between Version and Dependency. I think you simplify the code if you separate it more cleanly.

What do you mean here specifically?

• 6d616ee (Fix naming convention for the compiler handler classes and add documentation)
  • the changes look good, but:
  • dub.compilers.compiler and dub.compilers.[dmd|gdc|ldc] import each other.
    Break the import cycle by moving all but the dub.compilers.compiler.Compiler
    interface to dub.compilers.package. Maybe rename dub.compilers.compiler
    to dub.compilers.interfaces
  • Now I saw you did some of that in 5b2f303, but there are still a couple of things left

The only reason dub.compilers.compiler imports the implementations is to register them at startup. That code could for example also be moved to some higher level module (e.g. dub.dub).

• 5b2f303 (Move utility functions from dub.compilers.compiler to dub.compilers.utils)
  • the changes look good, but:
  • there are still some misc stuff left in dub.compilers.compiler that shouldn't be there

There is getCompiler, registerCompiler and defaultCompiler. The latter the only one that doesn't fully fit, but the utils module is intended to contain functionality that is useful to compiler class implementations, not for external code. A possibility would be to move this function to the Dub class (which is the only user so far and already exposes a defaultCompiler property).

• Move the isLinkerFile unittest from dub.generators.build to dub.compilers.utils

Done.

[s-ludwig]

Hm that test failure for 2d8f991 is a mystery...

edit: Looks like Travis-CI just picked up a newer commit when building... I was always under the impression that they'd do a git checkout of the actual commit?

[coveralls]

Coverage decreased (-0.5%) to 60.229% when pulling bff3355 on api-cleanup into 494acb7 on master.

[coveralls]

Coverage decreased (-0.5%) to 60.229% when pulling bff3355 on api-cleanup into 494acb7 on master.

[s-ludwig]

Cyclic compiler imports and defaultCompiler are also done now.

[s-ludwig]

Okay, since the remaining points are of general nature and not specific to the change set here, I'll merge this as soon as Travis is green to unblock further development, and will create an issue for the rest.

[coveralls]

Coverage decreased (-0.6%) to 60.202% when pulling 7dfb315 on api-cleanup into 494acb7 on master.