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

Complete object option encapsulation #38

Merged

Conversation

JannesBrands
Copy link
Member

Here my completion of the encapsulation of each object option byte into an specific struct. Fast reviews are welcome to merge this fast.

@GwnDaan
Copy link
Member

GwnDaan commented Nov 25, 2023

Hey @JannesBrands, the test case read_working_set_test seems to fail in Github Actions, could you take a look at that?

@JannesBrands
Copy link
Member Author

JannesBrands commented Nov 25, 2023

Hey @JannesBrands, the test case read_working_set_test seems to fail, could you take a look at that?

It's a todo. For that reason it's failing. The actual test is working.

not yet implemented: Test working set object with invalid data

I wanted to implement that inside the other PR that you merged last week. So now I'm completing in more little steps the things that were missing.

The unit test of "read working set" does not belong directly to this PR. I'll do another later to complete the testing.

@JannesBrands
Copy link
Member Author

So I could fix that inside this PR, but since this PR is about something else I would like to keep it inside a new PR. @GwnDaan let me know your opinion, if not, I'll fix it here.

@GwnDaan
Copy link
Member

GwnDaan commented Nov 25, 2023

I wanted to implement that inside the other PR that you merged last week.

That PR has been "unmerged", so basically this PR is that one + any additions you made since then. I don't mind a failing test for now since the target branch is not develop

@JannesBrands
Copy link
Member Author

I wanted to implement that inside the other PR that you merged last week.

That PR has been "unmerged", so basically this PR is that one + any additions you made since then. I don't mind a failing test for now since the target branch is not develop

Ahhhhhh, my bad. I didn't understand that point or better said: I forgot what actually happened at the end with the PR and merge haha.

Let me change the description and title later.
I'll hit you up when I complete something significant here.

@GwnDaan
Copy link
Member

GwnDaan commented Dec 4, 2023

Merging this now to allow us to faster iterate on it in main 😄

@GwnDaan GwnDaan merged commit 78f6b8a into Open-Agriculture:tjd/object-pool Dec 4, 2023
2 of 3 checks passed
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