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

Squash dict add test tags #145

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Squash dict add test tags #145

wants to merge 27 commits into from

Conversation

Juan-M-V
Copy link
Contributor

@Juan-M-V Juan-M-V commented Nov 16, 2022

Depends on #118

Add missing TEST comment to some hints in squash dict.

@codecov-commenter
Copy link

Codecov Report

Merging #145 (e161553) into main (b4674ea) will decrease coverage by 6.51%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##             main     #145      +/-   ##
==========================================
- Coverage   73.23%   66.71%   -6.52%     
==========================================
  Files          11       12       +1     
  Lines         609      679      +70     
==========================================
+ Hits          446      453       +7     
- Misses        163      226      +63     
Impacted Files Coverage Δ
src/lib.rs 0.00% <ø> (ø)
src/memory_segments.rs 96.96% <ø> (ø)
src/relocatable.rs 94.28% <0.00%> (-5.72%) ⬇️
src/dict_manager.rs 3.70% <3.70%> (ø)
src/ids.rs 33.59% <22.22%> (-0.87%) ⬇️
src/cairo_runner.rs 72.56% <100.00%> (+0.36%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/ids.rs Outdated Show resolved Hide resolved
fmoletta
fmoletta previously approved these changes Nov 28, 2022
Comment on lines +103 to +106
self.hint_locals.insert(
"__dict_manager".to_string(),
PyDictManager::new().into_py(py),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just take the GIL here rather than run everything with it taken.

Comment on lines +144 to +145
key: PyMaybeRelocatable,
val: PyMaybeRelocatable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not always a BigInt here?

vm: Rc<RefCell<VirtualMachine>>,
hint_value: Relocatable,
pub hint_value: Relocatable,
Copy link
Contributor

Choose a reason for hiding this comment

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

pub(crate)?

@@ -10,7 +10,7 @@ use std::{cell::RefCell, rc::Rc};

#[pyclass(name = "MemorySegmentManager", unsendable)]
pub struct PySegmentManager {
vm: Rc<RefCell<VirtualMachine>>,
pub(crate) vm: Rc<RefCell<VirtualMachine>>,
#[pyo3(get)]
memory: PyMemory,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's got the VM already, do we really need to keep this instance around?

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.

4 participants