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

Build arg setter for position #4396

Merged
merged 5 commits into from
Mar 11, 2024
Merged

Build arg setter for position #4396

merged 5 commits into from
Mar 11, 2024

Conversation

thomtrp
Copy link
Contributor

@thomtrp thomtrp commented Mar 11, 2024

Position input needs to be of type : number | first | last:

  • if number, returns value
  • if first, fetch first element ascending and set position / 2
  • if last, fetch first element descending and set position + 1

This logic is integrated into a new factory call arg-setters. It iterates recursively on all arguments and override existing ones using setter function.

import { FieldMetadataType } from 'src/metadata/field-metadata/field-metadata.entity';

@Injectable()
export class ArgsSettersFactory {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a big fan of the name since it does not "set" existing args but rather build new ones. But I don't know how I should call it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, maybe we can just call it ArgFactory for now ?
cc @charlesBochet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed QueryRunnerArgFactory so we avoid mistakes between existing arg factories in the builder

Copy link
Member

@magrinj magrinj left a comment

Choose a reason for hiding this comment

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

Small comments :)

import { FieldMetadataType } from 'src/metadata/field-metadata/field-metadata.entity';

@Injectable()
export class ArgsSettersFactory {
Copy link
Member

Choose a reason for hiding this comment

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

I agree, maybe we can just call it ArgFactory for now ?
cc @charlesBochet

return this.applySettersToArgsRecursive(args, options, fieldMetadataMap);
}

private async applySettersToArgsRecursive(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure of the naming too, maybe computeArgs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed createArgsRecursive

Copy link
Member

@magrinj magrinj left a comment

Choose a reason for hiding this comment

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

LGTM !

@thomtrp thomtrp merged commit c82c60b into main Mar 11, 2024
15 checks passed
@thomtrp thomtrp deleted the tt-build-position-setter branch March 11, 2024 15:18
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