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: Refine scaffolding for maintenance actions (DEV-3532) #3206

Conversation

BalduinLandolt
Copy link
Contributor

@BalduinLandolt BalduinLandolt commented Apr 23, 2024

Pull Request Checklist

Task Description/Number

This PR is mostly strictly refactoring: Moving code to separate file, etc. The most significant changes are:

  • removal of an unused dependency (PredicateObjectMapper) and its transitive dependencies
  • introducing a trait for maintenance actions (let me know if you find that useful at all!)
  • using the actual string values of the path variables for the different maintenance actions endpoints as examples in the OpenAPI docs.

PR Type

  • build/chore: maintenance tasks (no production code change)
  • docs: documentation changes (no production code change)
  • feat: represents new features
  • fix: represents bug fixes
  • perf: performance improvements
  • refactor: represents production code refactoring
  • test: adding or refactoring tests (no production code change)
  • deprecated: Deprecation warning (ideally referencing a migration guide)

Basic requirements for bug fixes and features

  • Tests for the changes have been added
  • Docs have been added / updated

Does this PR introduce a breaking change?

  • Yes

Does this PR change client-test-data?

  • Yes

Copy link

linear bot commented Apr 23, 2024

@BalduinLandolt BalduinLandolt force-pushed the feature/dev-3532-add-template-for-db-migrations-to-dsp-api-for-discussion branch from 76b7349 to ad0a19f Compare April 23, 2024 14:59
@BalduinLandolt BalduinLandolt marked this pull request as ready for review April 24, 2024 07:37
*
* @param params optional parameters as provided by the caller
*/
def execute(params: Option[A] = None): Task[Unit]
Copy link
Contributor

@seakayone seakayone Apr 24, 2024

Choose a reason for hiding this comment

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

idea: If we would type the params as A you could use Any for A to indicate that no params are necessary. This saves us from double checking in the execute method if params is a Some.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in a26c378, thanks for the hint, looks much nicer.

We can then type it as Any or Nothing, both seem to work.
The only downside is that I cannot provide a default argument anymore, so instead of calling FooAction(null).execute(), I have to do FooAction.execute(()). That's slightly less intuitive, but I think it's ok.
One option to get this to work might be to have a trait NoParamMaintenanceAction that constraints the type... but I don't think it's worth it.

@BalduinLandolt BalduinLandolt enabled auto-merge (squash) April 24, 2024 16:42
@BalduinLandolt BalduinLandolt merged commit 946de4e into main Apr 24, 2024
11 checks passed
@BalduinLandolt BalduinLandolt deleted the feature/dev-3532-add-template-for-db-migrations-to-dsp-api-for-discussion branch April 24, 2024 16:50
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