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

feat: add support for creating and updating file contents #148

Conversation

vootelerotov
Copy link
Contributor

See https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#create-or-update-file-contents.

I applied the well known engineering principle of "monkey-see-monkey-do". Hopefully my grasp of how to do things in this project is not off too much. Checkstyle yelling at me was definitely helpful.

Happy to do smaller follow-ups :)

@GithubStyle
@JsonSerialize(as = ImmutableFileCreate.class)
@JsonDeserialize(as = ImmutableFileCreate.class)
public interface FileCreate {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with request object, not sure where do you draw the line for sensible amount of parameters. Was on the fence with 3 here, with 4 for FileUpdate it seemed more clear cut.

import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;
import uk.co.datumedge.hamcrest.json.SameJSONAs;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw it on classpath, okay to use? Do not want to worry about difference in whitespace/order in mock and real-deal.

@vootelerotov
Copy link
Contributor Author

Bumping this up, anything I can do from my side to get this merged in?

@vootelerotov
Copy link
Contributor Author

@dziemba, could you please tell me if there is anything else I could do to get this PR rolling?

Not fond of pulling in one specific person, but starting to think that this PR might have fell through the cracks :)

@ebk45
Copy link
Contributor

ebk45 commented Dec 22, 2023

Hi @vootelerotov.

We apologise for taking so long to get eyes on this PR, we haven't been able to maintain this library to the standard we would have liked to but this will be changing in the new year. If you could please get this rebased and we'll be prioritising all open PRs ready for review.

Thanks!

Ellie

@vootelerotov vootelerotov force-pushed the feat/add-support-for-create-and-update-file-contents branch from 17cc521 to c0e19bf Compare January 2, 2024 19:58
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1602258) 76.86% compared to head (c0e19bf) 77.00%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #148      +/-   ##
============================================
+ Coverage     76.86%   77.00%   +0.13%     
- Complexity      297      299       +2     
============================================
  Files            42       42              
  Lines          1003     1009       +6     
  Branches         44       44              
============================================
+ Hits            771      777       +6     
  Misses          207      207              
  Partials         25       25              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vootelerotov
Copy link
Contributor Author

Hey @ebk45

Re-based the PR, it went rather smoothly.

I also noticed that there is another open PR (opened roughly a year ago) here that aims to do the same thing as this one. I did not see it before.

Let me know if there is anything else I can do.

@vootelerotov
Copy link
Contributor Author

we haven't been able to maintain this library to the standard we would have liked to

Do not be too demanding with yourself, it takes a lot of work to maintain an OSS library, especially supporting features that you are not using yourself. I have used this wrapper in multiple small projects, and I have found it useful as-is.

but this will be changing in the new year.
But glad to hear this :)

Copy link
Contributor

@dennisgranath dennisgranath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@dennisgranath dennisgranath merged commit 832fe33 into spotify:master Jan 12, 2024
5 checks passed
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.

3 participants