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: simplify dependency version resource generation #2849

Conversation

vitorhugods
Copy link
Member

@vitorhugods vitorhugods commented Apr 3, 2024


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

  1. DependenciesVersionTask doesn't rely on Gradle's input/output properties. Inputs and Outputs tell Gradle whether a task should be executed again or if it it already up-to-date.
  2. Maybe it knows a bit too much about Kalium, where these dependencies are defined, and where it should write the JSON to.
  3. It is a bit over complicated, by parsing the version catalog manually

Solutions

  1. Rewrite it to be an abstract class and make it use Input/Output. By doing this, Gradle will only run the task again if the output file was altered, or the inputs have changed.
  2. Make the task itself simpler: its only responsibility is to write a Map<String, String?> to a specified Json file. When registering the task, the caller can link it to Kalium and to the app's asset directory.
  3. The caller can figure it out by just accessing the version catalog instead of having to parse the toml file.

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Copy link
Contributor

github-actions bot commented Apr 3, 2024

Test Results

12 tests   12 ✅  0s ⏱️
 3 suites   0 💤
 3 files     0 ❌

Results for commit adb3699.

♻️ This comment has been updated with latest results.

@AndroidBob
Copy link
Collaborator

Build 3926 failed.

@AndroidBob
Copy link
Collaborator

Build 3927 failed.

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

👌

@vitorhugods vitorhugods enabled auto-merge (squash) April 3, 2024 12:39
@vitorhugods vitorhugods merged commit afb374b into release/candidate Apr 3, 2024
9 of 11 checks passed
@vitorhugods vitorhugods deleted the refactor/simplify-dependency-version-declaration branch April 3, 2024 13:01
Copy link
Contributor

github-actions bot commented Apr 3, 2024

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 3936 failed.

vitorhugods added a commit that referenced this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants