YAML parser for Dependency and Target #306
Conversation
There was a problem hiding this comment.
Thanks for tackling this! This is moving into the right direction.
I made a thorought thinking about how they should behave. For example I believe they should only represent SPEC, thus merely hold the values they're meant to (skipping extraneous keys, ...), and be isolated from the rest of Shards (no requirement on resolvers, configuration variables, ...).
Note: it doesn't build anymore.
src/dependency.cr
Outdated
| end | ||
|
|
||
| def version | ||
| version { "*" } | ||
| version? || "*" |
There was a problem hiding this comment.
Why not keep the yielding method?
There was a problem hiding this comment.
What's the benefit of a yielding method over the nilable? Usage is value { in_case_of_nil } vs value? || in_case_of_nil. The latter is more descriptive.
There was a problem hiding this comment.
It avoids the nilable union type. This is common in stdlib.
There was a problem hiding this comment.
A nilable union would still exist unless src/command/check.cr:45 is refactored.
There was a problem hiding this comment.
I still stand to my original opinion to have explicit keys for resolvers.
I'd reduce them to git and path with the parser immediately rewriting the github, bitbucket and gitlab resolvers as a Git URL, raising whenever trying to define git or path multiple times (or concurrently).
That would allow to drop the useless GithubResolver, BitbucketResolver and GitlabResolver classes, yet keep Dependency as close as possible to its SPEC definition.
|
I suppose having only The specialized classes could be removed regardless of this change, though. |
straight-shoota
force-pushed
the
feature/yaml-parsing
branch
from
9de0065 to
dba2846
Compare
straight-shoota
force-pushed
the
feature/yaml-parsing
branch
from
de8f216 to
e9a0f2b
Compare
|
Rebased on master |
straight-shoota
force-pushed
the
feature/yaml-parsing
branch
from
e9a0f2b to
3e60d10
Compare
|
Rebased again. |
* Refactor Dependency and Target with proper YAML parser * Refactor git and path properties on Dependency * Remove specialized git resolvers (GitHub, GitLab, BitBucket) in favour of handling generic git urls
* Refactor Dependency and Target with proper YAML parser * Refactor git and path properties on Dependency * Remove specialized git resolvers (GitHub, GitLab, BitBucket) in favour of handling generic git urls
Implements proper YAML parses for Dependency and Target to avoid relying on a hash for storing the mappings. Instead, these types use specific properties now.
Extra attributes are still collected in a hash. This has no real function currently, but it allows for future enhancements with new keys not causing the current implementation to fail reading the data.
/cc #230