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

Implement the repository URL #3192

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Implement the repository URL #3192

wants to merge 14 commits into from

Conversation

Garanas
Copy link
Member

@Garanas Garanas commented Jun 2, 2024

Relates to:

It's shown as the 'Source' link, below the author. Visually it is not ideal, but we can improve that separately.

image

Comment on lines 340 to 342
// retrieve information from the mod_info.lua to send to the API
ModVersion modVersion = extractModInfo(modPath);
modUploadTask.setRepositoryURL(modVersion.mod().repositoryURL());
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved into the ModUploadTask itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Processed in 26166cb

Comment on lines 81 to 84
// retrieve information from the mod_info.lua to send to the API

ModVersion modVersionInfo = modService.extractModInfo(modPath);
URL repositoryURL = modVersionInfo.mod().repositoryURL();
Copy link
Member

Choose a reason for hiding this comment

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

This is shadowing the field repositoryUrl now. So the field should be removed or this should set the field if it is null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Processed in: 57ff0b3

Copy link

codecov bot commented Jun 2, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 58.50%. Comparing base (dd9ab2b) to head (5def4c6).
Report is 25 commits behind head on develop.

Current head 5def4c6 differs from pull request most recent head 23ab26f

Please upload reports for the commit 23ab26f to get more accurate results.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #3192      +/-   ##
=============================================
- Coverage      58.83%   58.50%   -0.33%     
- Complexity      3984     4085     +101     
=============================================
  Files            576      581       +5     
  Lines          19296    19791     +495     
  Branches        1022     1035      +13     
=============================================
+ Hits           11353    11579     +226     
- Misses          7447     7707     +260     
- Partials         496      505       +9     
Files Coverage Δ
...main/java/com/faforever/client/domain/api/Mod.java 100.00% <ø> (ø)
...va/com/faforever/client/domain/api/ModVersion.java 100.00% <ø> (ø)
...java/com/faforever/client/mapstruct/ModMapper.java 0.00% <ø> (ø)
...n/java/com/faforever/client/mod/ModUploadTask.java 94.59% <75.00%> (+4.59%) ⬆️
.../com/faforever/client/mod/ModDetailController.java 81.95% <20.00%> (-5.04%) ⬇️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@Garanas Garanas marked this pull request as ready for review June 2, 2024 18:40
@Garanas Garanas requested a review from Sheikah45 June 2, 2024 18:41
Comment on lines 170 to 171
runOnFxThreadAndWait(() -> instance.setModVersion(modVersion));
WaitForAsyncUtils.waitForFxEvents();
Copy link
Member

Choose a reason for hiding this comment

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

technically you don't need WaitForAsyncUtils.waitForFxEvents if you run things with runOnFxThreadAndWait

Copy link
Member Author

Choose a reason for hiding this comment

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

Processed in 23ab26f

Copy link
Member

@Sheikah45 Sheikah45 left a comment

Choose a reason for hiding this comment

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

Looking at this I think we may want to check the repository url before uploading the mod. The API only accepts certain hosts as the repository url for a mod. And we could probably give a better error message if we detect it earlier.

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.

None yet

2 participants