Skip to content

Move automodeling logic out of view#2680

Merged
charisk merged 2 commits intomainfrom
charisk/move-automodeling
Aug 9, 2023
Merged

Move automodeling logic out of view#2680
charisk merged 2 commits intomainfrom
charisk/move-automodeling

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Aug 8, 2023

Moved logic out of the data extensions editor view and into a new AutoModeler class.

This will make it easier to introduce batching and cancellation.

There are no material changes in this PR, code has mostly been copy pasted with minor edits.

Checklist

N/A:

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested a review from starcke August 8, 2023 15:59
@charisk charisk requested a review from a team as a code owner August 8, 2023 15:59
@charisk charisk changed the title Charisk/move automodeling Move automodeling logic out of view Aug 8, 2023
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

Looks good to me.

) => Promise<void>,
) {}

public async startModeling(
Copy link
Copy Markdown
Contributor

@starcke starcke Aug 9, 2023

Choose a reason for hiding this comment

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

Is this method to prepare for the future, because it doesnt seem to do much other than just call the other method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. It'll be doing a bit more later on.

import { DatabaseItem } from "../databases/local-databases";
import { Mode } from "./shared/mode";

export class AutoModeler {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add some comments to the methods in here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. Did you have anything specific in mind that you wanted commented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I might just do that in a separate PR when there's a bit more beef to it. But please let me know if you'd like anything specific commented :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe what some of the parameters should contain? We can also do it in a followup.

@charisk charisk enabled auto-merge August 9, 2023 08:32
@charisk charisk merged commit 6dc0b14 into main Aug 9, 2023
@charisk charisk deleted the charisk/move-automodeling branch August 9, 2023 11:00
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.

2 participants