Remove weird constructors #3261
Conversation
|
I think it would be nicer not to introduce all the new types, instead just use a function |
barucden
force-pushed
the
remove-dict-ctor
branch
3 times, most recently
from
1b0e16e to
bf7163e
Compare
|
I did as suggested. |
|
The failed test is due to a network error during a git clone. |
|
What do you think, @fredrikekre? Is it ready for merge? (cc @KristofferC, who reported the issue) |
|
Yep looks good, thank you! |
|
Actually, could the |
I believe so. It just appeared to me like the perfect use-case for an abstract type with several subtypes. It seemed cleaner to me. Is there a particular reason for Union? |
When there are only a few "subtypes" it means the compiler can branch on the different unions. If it is an abstract type the compiler doesn't know anything. It might not matter a lot here but there has been some issues earlier with "invalidations" (a bit of a technical thing) when Pkg has some type unstable code. To prevent that, I think maybe we should just put back the union so that things are the way they used to be. |
barucden
force-pushed
the
remove-dict-ctor
branch
from
bf7163e to
ed413be
Compare
|
I was not aware of that; thanks for the explanation. I still left |
`APIOptions` is defined as an alias for `Dict{Symbol, Any}`.
Consequently, the definition of a constructor for `APIOptions`
introduced a new constructor for Dict. This commit removes that
constructor.
`PackageToken` is a Union of multiple struct types. This commit removes a constructor for `PackageToken` and replaces it with a function `packagetoken`. Furthermore, this commit removes `PackageIdentifier` as an alias for String and replaces that with a struct type, which wraps a String containing a package identifier.
barucden
force-pushed
the
remove-dict-ctor
branch
from
ed413be to
e50d8bf
Compare
Resolves #3215
The individual commits are standalone, one addresses
APIOptionsand the otherPackageToken, so they don't have to be squashed.