Skip to content

Revert #120 "Prevent variable nullification on later imports" - #124

Open
m-dzianishchyts wants to merge 6 commits into
dflib:mainfrom
m-dzianishchyts:119-fix-variable-not-updating-on-redeclaration
Open

Revert #120 "Prevent variable nullification on later imports"#124
m-dzianishchyts wants to merge 6 commits into
dflib:mainfrom
m-dzianishchyts:119-fix-variable-not-updating-on-redeclaration

Conversation

@m-dzianishchyts

@m-dzianishchyts m-dzianishchyts commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Reverts #120 and updates integration test cases reproducing the reported issues.

@m-dzianishchyts
m-dzianishchyts marked this pull request as ready for review June 24, 2026 16:00
@m-dzianishchyts
m-dzianishchyts marked this pull request as draft June 26, 2026 12:36
@m-dzianishchyts m-dzianishchyts changed the title Implement JVM HotSwap via Java Agent Revert #120 "Prevent variable nullification on later imports" Jul 6, 2026
@m-dzianishchyts
m-dzianishchyts marked this pull request as ready for review July 17, 2026 12:47
@andrus

andrus commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@m-dzianishchyts , @stariy95 : guys, I tried to understand how this PR is different from the original #120. So I manually reverted all #120 commits (already pushed to "main") and then did a merge --squash of this one on top. The only change that I see is introduction of KernelExecutionIT.java test. But the original problem is still there when I run it manually. It was not fixed.

@andrus

andrus commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Ok, I was able to rewrite KernelExecutionIT in a way that reproduces the issue. It is committed to main (see a @Disabled test). The reason the original test didn't fail was that each line was executed in a separate cell. Somehow combining things in cells in a certain way causes this problem. Here are Claude findings on which combos work and which don't:

  ┌──────────────────────────────────────────────────────────┬────────┐
  │                          cells                           │ result │
  ├──────────────────────────────────────────────────────────┼────────┤
  │ 5 separate cells (what the IT does)                      │ ✅ ok  │
  ├──────────────────────────────────────────────────────────┼────────┤
  │ all 5 in one cell                                        │ ❌ NPE │
  ├──────────────────────────────────────────────────────────┼────────┤
  │ %maven alone, then remaining 4 lines in one cell         │ ❌ NPE │
  ├──────────────────────────────────────────────────────────┼────────┤
  │ %maven / import+var om+import / om.getClass()            │ ❌ NPE │
  ├──────────────────────────────────────────────────────────┼────────┤
  │ %maven / import+var om / import+om.getClass()            │ ✅ ok  │
  └──────────────────────────────────────────────────────────┴────────┘

One little thing left - figure out how to fix it 🙂

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