Skip to content

feat: database user controller - #102

Merged
mikededo merged 19 commits into
mainfrom
feat/user_controller_from_db
Dec 18, 2022
Merged

feat: database user controller#102
mikededo merged 19 commits into
mainfrom
feat/user_controller_from_db

Conversation

@mikededo

Copy link
Copy Markdown
Contributor

No description provided.

@mikededo mikededo added type: feat New feature or request status: wip PR work in progress labels Dec 13, 2022
@mikededo mikededo added this to the 2 - MVP milestone Dec 13, 2022
@mikededo
mikededo requested a review from a team December 13, 2022 10:03
@mikededo

Copy link
Copy Markdown
Contributor Author

I created this PR so that we can discuss what is being develop.

  • I initially proposed that minor PR's were made regarding the controllers so that we could keep track of what was being developed. All these PR's would be merged to main since it is code that would not be used. This is the reason why we merged the feat: merge teams into pull requests #73 and now we are working with smaller PR's as explained in feat: UserOptionController (use only data from the external API - skip DB) #70.
  • I do not totally agree of adding a controller folder at the same root of the shell and github folders. With this, I am also realising that we are probably coupling the UserOptionController to the shell infra, but then we are using the GitHub concrete implementations. We should probably re-think where to locate the controllers.
  • In the UserOptionController, we have been using for(T t : Ts) loops, instead of lambdas in the forEach. We should be consistent and only use one of the both. I don't see the need of why it has been done different in this part.
    If it is preferred using the forEach method on the array, then create an issue if possible that requestes this changes.

@sdomingobasora

Copy link
Copy Markdown
Collaborator

I added a few commits.
At this point, I'm in favor of returning to small PRs, so I would like for this to get merged in order to resume development on the new database user controller, the Proxy piece, the aggregation orchestrator and so on.

@sdomingobasora

Copy link
Copy Markdown
Collaborator

@github-stats-22/gs-developers are you okey if we merge this, and make small PRs from now on? I know there are some merge conflicts, I can resolve them. As for what @mikededo said about the controller location, it's being discussed in #107 and there will be major changes afterwards, so I think it's okey if this PR does not respect criteria because we will change things later anyway.

@plozanol
plozanol marked this pull request as ready for review December 18, 2022 00:19
@mikededo
mikededo merged commit 381cc25 into main Dec 18, 2022
@mikededo
mikededo deleted the feat/user_controller_from_db branch December 18, 2022 01:40
@mikededo mikededo added status: done PR/Issue completed and removed status: wip PR work in progress labels Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: done PR/Issue completed type: feat New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants