Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to move github/data to vcs/data #497

Merged

Conversation

daddykotex
Copy link
Contributor

This is mostly changes to prevent the PR with the actual logic to be obfuscated. I move the data structures from github/data to vcs/data. I also rename the GitHubRepoAlg to VCSRepoAlg.

With this PR, we are assuming the data structures (used to talk to GitHub) will work for any VCS be it BitBucket and GitLab. In reality, it's a bit of a stretch. I've made it work with a bunch of shortcut (as a prototype it ran on one of our internal repository and it opened a bunch of PR 👍 ). But we'll need to discuss what needs to change in those structures in order to accommodate other VCS when I open the PR with the actual logic.

The actual changes would look like:

  • a new package called org.scalasteward.core.gitlab (with a different Url class
  • custom encoder/decoders to go from Gitlab's JSON to the vcs data structures (if they're left as-is)
  • a VCSApiAlg that covers Github and Gitlab
  • a VCS Selector (if you will) to read the config and build the appropriate implementation of VCSApiAlg

@daddykotex
Copy link
Contributor Author

Referencing #313

@codecov
Copy link

codecov bot commented May 19, 2019

Codecov Report

Merging #497 into master will not change coverage.
The diff coverage is 25%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #497   +/-   ##
=======================================
  Coverage   55.86%   55.86%           
=======================================
  Files          64       64           
  Lines         784      784           
  Branches       33       37    +4     
=======================================
  Hits          438      438           
  Misses        346      346
Impacted Files Coverage Δ
...ala/org/scalasteward/core/application/Config.scala 0% <ø> (ø) ⬆️
...alasteward/core/github/http4s/authentication.scala 100% <ø> (ø) ⬆️
...src/main/scala/org/scalasteward/core/steward.scala 0% <ø> (ø) ⬆️
...ore/dependency/json/JsonDependencyRepository.scala 0% <ø> (ø) ⬆️
...lasteward/core/nurture/json/PullRequestStore.scala 0% <ø> (ø) ⬆️
...a/org/scalasteward/core/update/UpdateService.scala 0% <ø> (ø) ⬆️
...la/org/scalasteward/core/application/Context.scala 0% <ø> (ø) ⬆️
...ala/org/scalasteward/core/vcs/data/CommitOut.scala 100% <ø> (ø)
.../main/scala/org/scalasteward/core/git/GitAlg.scala 53.57% <ø> (ø) ⬆️
...g/scalasteward/core/repoconfig/RepoConfigAlg.scala 100% <ø> (ø) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8483ca5...9aeacdc. Read the comment docs.

@fthomas
Copy link
Member

fthomas commented May 26, 2019

Ok, let's do this! I agree that reusing the same data structures for different repository mangers might be problematic. Especially the Out classes currently mirror GitHub's API and if we want to support different features for different repository mangers in the future we probably also need different Out classes.

Btw, thanks @daddykotex for splitting your changes into this "rename" PR and a later "logic" PR. This makes it easier for me to review them.

@fthomas fthomas merged commit f098c09 into scala-steward-org:master May 26, 2019
@fthomas fthomas added this to the 0.3.0 milestone May 26, 2019
@daddykotex
Copy link
Contributor Author

Wow amazing. Thanks to you for scala-steward.

If you want to make sure you are not breaking anything, we can work on a feature branch. I'll open the follow-up MR and explains the difference so we can all be on the same page about the differences between GitHub integration and Gitlab integration.

@fthomas
Copy link
Member

fthomas commented May 26, 2019

👍 Sounds good to me. I've created the branch topic/gitlab for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants