♻️ Refactor last_cell check & consecutiveness check in publish()

[falexwolf]

Addresses

• Integrate last_cell check with consecutiveness check #149

This also makes the nutshell a lot prettier, as the weird test CI env exceptions disappeared.

[Koncopd]

But this makes impossible to run meta.live.integrity from header, no? We discussed addition of integrity check to header.
I also think that integrity and last cell checks are different things and we should be able to use them separately, like in header live field for integrity. I really would prefer to have them separate. What is the problem of calling 2 functions instead of one where we need 2 checks?

[falexwolf]

Right, we need to be able to run it in header()!

I'll shift things around!

[falexwolf]

The important thing to me is that there is no code redundancy in calling_statement in publish(). And it'd also be nice if ignore_code wasn't necessary.

[Koncopd]

But we haven't discussed this and i really don't understand why we need to unify this. Having 2 specific functions is just easier for development and it is not a user facing thing.

[Koncopd]

I don't like the approach of one function with specific exceptions for header.

[falexwolf]

Yes, yes! It was always two functions, and now they're not coupled together anymore! 😅

[Koncopd]

But now it is impossible to do integrity check without last cell check.

[falexwolf]

Take a look at the code and you'll see it's possible!

[falexwolf]

I agreed with you immediately: #167 (comment)

Apologies for my oversight!

[Koncopd]

Ok, but then you need to revert deleting ignore_code, i don't think it will work without it.

[codecov]

Codecov Report

Merging #167 (cb4b101) into main (d85f41a) will increase coverage by 0.57%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
+ Coverage   81.10%   81.68%   +0.57%     
==========================================
  Files          17       18       +1     
  Lines         794      797       +3     
==========================================
+ Hits          644      651       +7     
+ Misses        150      146       -4     

Impacted Files	Coverage Δ
nbproject/_meta.py	82.35% <71.42%> (+4.30%)	⬆️
nbproject/_publish.py	89.74% <100.00%> (+0.38%)	⬆️
nbproject/dev/_check_last_cell.py	100.00% <100.00%> (ø)
nbproject/dev/_consecutiveness.py	95.23% <100.00%> (-0.22%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c246317...cb4b101. Read the comment docs.

[github-actions]

🚀 Deployed on https://62d28f2e4624d471cf15427b--nbproject-3aa23rammb.netlify.app

[falexwolf]

Ok, but then you need to revert deleting ignore_code, i don't think it will work without it.

You mean because the cell that executes the statement has by definition None as the execution number? 🤔

I'd see the header() use case along what we discussed. You have an executed notebook in front of you, and you're testing consecutiveness for that executed notebook from an arbitrary cell.

The first thing that should happen is to read in the state of the notebook from file in which all cells have execution numbers then, I think. And this should be possible without ignore_code, right? 🤔

[falexwolf]

I mean, I'm just testing it!

[Koncopd]

Yes, None execution number and non-empty source. Yes, for header it should not matter but for meta.live.integrity (or how it is called now) it matters in general.

[falexwolf]

Yes, this is indeed the mechanics.

I don't really like that one needs to provide ignore_code as that means

• anyone who writes a wrapper needs to update that part
• in principle there could be accidental matches (although the chance for this really is negligibly small)

But I also don't really see an alternative other than not calling _save_notebook, which would have disadvantages and would really just make meta.live.consecutive_cells viable for checking files on disk while executing header().

However, if this really is the only use-case, maybe we should just get rid of the field in meta.live! I found that it wasn't even covered by a test nor mentioned in the docs anyway.

So, maybe there is indeed no canonical use case and then the best would probably be to just simplify the API.

We can do that in another PR if we want.

[Koncopd]

I though on reading stack trace for this at some point, ignore_code was just a fast solution to this problem. Hm, i remeber we had a notebook with meta.live.integrity at some point, no? Can't find it now.

[falexwolf]

I though on reading stack trace for this at some point, ignore_code was just a fast solution to this problem. Hm, i remeber we had a notebook with meta.live.integrity at some point, no? Can't find it now.

I added one for now, which also increases coverage.

We can give this another thought. If we can simplify the code & API by dropping a feature that nobody would meaningfully use anyway, we should always do it.

The less possibilities for users to make mistakes, the better. The simpler the code, the easier to maintain.

[Koncopd]

Thank you. I guess we can drop that, as long as we have check_consecutiveness for development cases.

[falexwolf]

In any case - the logic almost didn't change in this PR, things got just moved around, the docs are prettier now, coverage increased a bit.

The only part where the logic changed is that the consecutiveness check called upon publish doesn't rely on ignore_code right now, but instead relies on check_cell having passed before.

[falexwolf]

Thank you. I guess we can drop that, as long as we have check_consecutiveness for development cases.

The current reason to keep is that if we print it in header(), the user will start to look for how this is computed. And then they won't find anything in meta. This is probably OK.

I'd also be in favor of dropping it.

I think the right PR to drop it is when you build in the check within header(), as we discussed yesterday.