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

Make Deps.kt the source of truth for dependencies #641

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Conversation

doodla
Copy link
Contributor

@doodla doodla commented Feb 25, 2020

Versions haven't been modified, so if this builds and passes tests, we should be good to go.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #641 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #641      +/-   ##
============================================
- Coverage     76.31%   76.22%   -0.09%     
  Complexity      610      610              
============================================
  Files            81       81              
  Lines          2297     2297              
  Branches        327      327              
============================================
- Hits           1753     1751       -2     
- Misses          324      326       +2     
  Partials        220      220

@pawelpasterz
Copy link
Contributor

Hey @doodla could you change commits message to be more explanatory? Thanks :)

@doodla
Copy link
Contributor Author

doodla commented Feb 25, 2020

I have read the CLA Document and I hereby sign the CLA

@doodla
Copy link
Contributor Author

doodla commented Feb 25, 2020

Hey @doodla could you change commits message to be more explanatory? Thanks :)

It's too much work, imo. The PR title should be sufficient. I usually don't bother with detailed commit messages in repositories where the pull requests are merged by squashing.

@pawelpasterz
Copy link
Contributor

Hey @doodla could you change commits message to be more explanatory? Thanks :)

It's too much work, imo. The PR title should be sufficient. I usually don't bother with detailed commit messages in repositories where the pull requests are merged by squashing.

Got it, thanks for answer

@doodla
Copy link
Contributor Author

doodla commented Feb 25, 2020

Got it, thanks for answer

For some more context, my thinking is that Pull requests are atomic commits ( squash merge ) into the master branch. Because of that, it makes little sense to document exactly how you arrived there if the PR author doesn't feel the need to. In my case, the majority of the change is likely the first commit with all others being modifications based on what CI spits out. f is an abbreviation of fix for me.

If a pull request can be divided into multiple discrete chunks, each chunk can be a separate pull request to decrease the size and make the reviewers' lives easy.

@doodla doodla mentioned this pull request Feb 25, 2020
@jan-goral jan-goral self-requested a review February 26, 2020 00:20
@jan-goral jan-goral self-requested a review February 26, 2020 00:25
@bootstraponline bootstraponline merged commit 0aa0512 into master Feb 26, 2020
@bootstraponline bootstraponline deleted the sv/deps branch February 26, 2020 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants