Conversation
deploystream/apps/feature/lib.py
Outdated
There was a problem hiding this comment.
This may sound weird to you, but why don't we use a single dict that can have different types of keys, either strings when searching for provider name, or classes when searching for provider type. This is Python after all so we are not subject to single-type keys.
I see no danger in overloading the providers dictionary this way, and the code reads clearer.
|
Looking good. If you wish we can split the work in smaller tasks, as e.g. just the front-end feature detail view seems like quite some work. I would like this to look much nicer than storyboard, so it would be nice to try to sketch out beforehand what we want to display and how (as opposed to getting a bunch of stuff in a template and try to make it look decent as an afterthought). |
|
I think we should absolutely split the work into smaller tasks. I've updated the list of outstanding things in this PR and split them into front end / back end. By all means take the front end ones. One way would be to leave this PR open and if you're happy with it simply comment that you are and we then create PRs into this branch for the remaining work and we can therefore see when we're finished and be reminded of the outstanding subtasks... |
|
Good plan. |
There was a problem hiding this comment.
What do you think about this? It goes against the philosophy that I've been adopting to keep providers as dumb as possible.
An alternative would be that the provider defines a def iter_branches(self) method - returning repo name, branch name and head commit - and in https://github.com/pretenders/deploystream/pull/73/files#L0R54 we loop through each performing the relevant hierarchy.match_with_levels function.
The second approach would make test writing a lot easier too.
Which way do you prefer?
There was a problem hiding this comment.
I think I'm going to take the view that each source code provider is responsible for it's own level producing - and that they may have different ways of calculating that - so therefore leaving the call to match_with_levels inside github's provider.
There was a problem hiding this comment.
OK I haven't given this enough thought TBH.
Dev/70/alex
…m into story/70/alex
@txels This is far from complete, but wanted to let you see it. Any comments welcome.
This is the beginning of solving #70 and #71
Still needed at the back end to complete this:
An integration test to ensure we're getting the right hierarchies created.See Dev/70/alex #77Writing of theSee Dev/70/alex hierarchy #82create_hierarchy_trees(with a possible rename) to calculate which branches from each level are actually merged into higher ones and vice versa. This might end up needing to be moved to within a provider - or at least it will require extensive access to one.Front end work: