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

Feat: Follow up on source calls #609

Merged
merged 51 commits into from
Feb 7, 2024
Merged

Conversation

Ellpeck
Copy link
Member

@Ellpeck Ellpeck commented Jan 22, 2024

VERY work in progress, I have no idea what I'm doing (YET)

@Ellpeck Ellpeck linked an issue Jan 22, 2024 that may be closed by this pull request
11 tasks
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (d69018e) 71.34% compared to head (b5ddd9a) 71.64%.
Report is 5 commits behind head on main.

Files Patch % Lines
src/r-bridge/retriever.ts 86.20% 3 Missing and 1 partial ⚠️
src/dataflow/internal/process/functions/source.ts 95.55% 1 Missing and 1 partial ⚠️
src/cli/repl/commands/parse.ts 0.00% 1 Missing ⚠️
src/core/print/parse-printer.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
+ Coverage   71.34%   71.64%   +0.29%     
==========================================
  Files         217      218       +1     
  Lines        7005     7082      +77     
  Branches     1085     1094       +9     
==========================================
+ Hits         4998     5074      +76     
  Misses       1720     1720              
- Partials      287      288       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ellpeck
Copy link
Member Author

Ellpeck commented Jan 25, 2024

am I too stupid for a rebase, I click the "rebase" button and then it does this

@Ellpeck
Copy link
Member Author

Ellpeck commented Jan 25, 2024

@EagleoutIce as you can see, all regular tests will also fail with the same dataflow being undefined exception just due to the fact that the source parsing code exists - regardless of whether it's actually executed.

@EagleoutIce
Copy link
Member

First, I probably want to add "importsNotUsedAsValues": "error", to the tsconfig to prevent some of the possibilities (and deal with the upcoming errors) -> #616.

Besides it may very well be a problem of you now requiring the steps in the dataflow, while the steps require the dataflow. Can you check if something like https://github.com/import-js/eslint-plugin-import is sufficient to detect this? (and if so, add it in a new issue?).
Besides, can you break this cycle?

@Ellpeck
Copy link
Member Author

Ellpeck commented Jan 30, 2024

It looks like the no-cycle rule doesn't detect any issues with dependency cycles. I tried a few other, seemingly related rules, but none of them seemed to catch it either. Did you have a specific rule in mind?

@Ellpeck
Copy link
Member Author

Ellpeck commented Jan 30, 2024

import type did nothing to fix the cyclic dependency issue so that's just great

src/dataflow/processor.ts Outdated Show resolved Hide resolved
@Ellpeck
Copy link
Member Author

Ellpeck commented Feb 6, 2024

I think this is mostly finished? I might be missing something huge and just totally forgetting about it, but all the major items from the issue #605 are implemented and should be working according to the tests :)

@Ellpeck Ellpeck marked this pull request as ready for review February 6, 2024 12:16
@Ellpeck Ellpeck linked an issue Feb 6, 2024 that may be closed by this pull request
src/dataflow/internal/process/functions/function-call.ts Outdated Show resolved Hide resolved
src/dataflow/internal/process/functions/source.ts Outdated Show resolved Hide resolved
src/dataflow/internal/process/functions/source.ts Outdated Show resolved Hide resolved
src/dataflow/internal/process/functions/source.ts Outdated Show resolved Hide resolved
src/dataflow/internal/process/functions/source.ts Outdated Show resolved Hide resolved
src/dataflow/internal/process/functions/source.ts Outdated Show resolved Hide resolved
src/dataflow/processor.ts Show resolved Hide resolved
src/r-bridge/retriever.ts Show resolved Hide resolved
@EagleoutIce
Copy link
Member

Please add an afterEach hook (?) that clears the source provider so that we can be sure they do not accidentally leak into other tests (keep it local to the dataflow tests, that should suffice).

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.

Looks fine to me!

@EagleoutIce EagleoutIce merged commit ec104b8 into main Feb 7, 2024
21 checks passed
@EagleoutIce EagleoutIce deleted the 605-feat-follow-up-on-source-calls branch February 7, 2024 13:01
@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.

Handle cyclic imports and sources Feat: Follow up on source calls
2 participants