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

Simplify Dataflow Tests #665

Merged
merged 16 commits into from
Feb 24, 2024
Merged

Conversation

bjthehun
Copy link
Contributor

@bjthehun bjthehun commented Feb 20, 2024

Closes #158, when merged.

  • Introduces a DataflowGraphBuilder.
  • The builder has convenience methods to easily add vertices/edges of a given type.
  • These methods replace addVertex/addEdge in dataflow tests.
  • Also removed default/optional parameters.

src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
@EagleoutIce
Copy link
Member

Looks great so far!

Hopefully, I will be able to take a closer look in roughly 12-14 hours!

Thank you very much for your great work!

Copy link
Member

@EagleoutIce EagleoutIce left a comment

Choose a reason for hiding this comment

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

I had some time to review the current changes :)

Thank you very much!

src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
src/dataflow/graph/graph.ts Outdated Show resolved Hide resolved
Added explicitly instead of adding methods.
Also fix node types in method signatures.
Copy link
Member

@EagleoutIce EagleoutIce left a comment

Choose a reason for hiding this comment

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

Thank you for switching to the builder layer!

test/functionality/_helper/dataflowgraph-builder.ts Outdated Show resolved Hide resolved
@bjthehun bjthehun marked this pull request as ready for review February 23, 2024 18:43
@bjthehun
Copy link
Contributor Author

Dataflow graph building itself has been simplified. However, defining/using references and environments still is comparatively cumbersome.
I'd probably wait for the code review, plus #673, to work on that, because inconsistent spacing gave me quite some trouble while switching to the builder methods.

Copy link
Member

@EagleoutIce EagleoutIce left a comment

Choose a reason for hiding this comment

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

I was not able to find an error in the updated tests!

@EagleoutIce
Copy link
Member

EagleoutIce commented Feb 24, 2024

I have just implemented #673.

@bjthehun Do you want me to handle all the new conflicts resulting from the unified spacing?

@bjthehun
Copy link
Contributor Author

I have just implemented #673.

@bjthehun Do you want me to handle all the new conflicts resulting from the unified spacing?

If you want to, then yes.

@EagleoutIce EagleoutIce merged commit b840f09 into flowr-analysis:main Feb 24, 2024
13 checks passed
@EagleoutIce
Copy link
Member

This pull request is included in v1.4.2 (see Release v1.4.2 (Dropping xmlparsedata, Benchmark Re-Runs, and Repl Fixes)).

1 similar comment
@EagleoutIce
Copy link
Member

This pull request is included in v1.4.2 (see Release v1.4.2 (Dropping xmlparsedata, Benchmark Re-Runs, and Repl Fixes)).

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.

Make the Creation of Dataflow Graphs in the Tests More Readable
2 participants