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: bundle API #48

Merged
merged 11 commits into from
Jun 15, 2023
Merged

feat: bundle API #48

merged 11 commits into from
Jun 15, 2023

Conversation

Halle
Copy link
Contributor

@Halle Halle commented Jun 15, 2023

Greetings,

Due to our local requirements I added bundle export and bundle download to our fork, and wanted to offer this PR in case you also wanted to support them as a client feature. I've been using these methods in our automation for six weeks and it seems to function well, and similarly to the rest of the client. I can't offer to write more unit tests right now, but if someone else wants to contribute them to finish up the PR if that is an acceptance requirement, I welcome it.

Thanks,

Halle

@andrii-bodnar
Copy link
Member

Hi @Halle, thanks for the contribution!

Could you please address the failed CI/CD checks? https://github.com/crowdin/crowdin-api-client-ruby/actions/runs/5276119680/jobs/9542505611?pr=48

@andrii-bodnar
Copy link
Member

@Halle It would be also great to add the Check Bundle Export Status.

The Export Bundle, Check Bundle Export Status, and Download Bundle methods are supposed to work together.

The Download Bundle itself doesn't make a lot of sense because we need to check the export status, ensure that it's finished, and then we can finally download it.

Could you also add this method? Thanks!

@andrii-bodnar andrii-bodnar linked an issue Jun 15, 2023 that may be closed by this pull request
@Halle Halle changed the title Bundle export and bundle download API feat: bundle API Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #48 (b0b68f3) into main (5cd45c5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head b0b68f3 differs from pull request most recent head 2709ff6. Consider uploading reports for the commit 2709ff6 to get more accurate results

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   97.92%   97.96%   +0.04%     
==========================================
  Files          57       57              
  Lines        2643     2683      +40     
==========================================
+ Hits         2588     2628      +40     
  Misses         55       55              
Impacted Files Coverage Δ
lib/crowdin-api/api_resources/bundles.rb 100.00% <100.00%> (ø)
spec/api_resources/bundles_spec.rb 100.00% <100.00%> (ø)
spec/unit/client_spec.rb 100.00% <100.00%> (ø)

@andrii-bodnar
Copy link
Member

andrii-bodnar commented Jun 15, 2023

@Halle thanks for the PR update. There are still a couple of CI issues, please take a look

@Halle
Copy link
Contributor Author

Halle commented Jun 15, 2023

@Halle thanks for the PR update. There are still a couple of CI issues, please take a look

Hi @andrii-bodnar, it isn't actually clear to me why the two rspec failures are occurring. Do you see it and can you make a code suggestion?

@andrii-bodnar
Copy link
Member

andrii-bodnar commented Jun 15, 2023

@Halle currently, I see only the failing check regarding trailing whitespaces. See my code suggestion - #48 (comment)

@Halle
Copy link
Contributor Author

Halle commented Jun 15, 2023

Hi @andrii-bodnar, sorry, adding and real-world-testing check_bundle_export_status used up my time budget for this PR, so I'm going to have to leave these last few documentation changes for you. I hope this has been helpful for meeting the goal of your bundles API enhancement!

@andrii-bodnar
Copy link
Member

@Halle thank you!

lib/crowdin-api/api_resources/bundles.rb Outdated Show resolved Hide resolved
lib/crowdin-api/api_resources/bundles.rb Outdated Show resolved Hide resolved
lib/crowdin-api/api_resources/bundles.rb Outdated Show resolved Hide resolved
@andrii-bodnar andrii-bodnar merged commit c78215c into crowdin:main Jun 15, 2023
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.

Bundles API enhancement
2 participants