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(systemextensionsctl): add functionality #597

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

georgettica
Copy link
Contributor

No description provided.

@georgettica
Copy link
Contributor Author

test failed on ubuntu even though this was only set to darwin?

@kellyjonbrazil kellyjonbrazil changed the base branch from master to dev September 25, 2024 18:37
@kellyjonbrazil
Copy link
Owner

I just had to change the base branch to dev. There are some python backports I had to work around and the fixes are in that branch.

@georgettica
Copy link
Contributor Author

Oops, didn't check that :/
Thanks for changing!

@georgettica
Copy link
Contributor Author

On my side I am ready to merge, if tests pass (and they did)

@kellyjonbrazil
Copy link
Owner

Would be good to have some tests before merging, if possible.

@georgettica
Copy link
Contributor Author

legitimate bug in code, caught by tests. need to mull it over until I fix it, or if you know how to resolve the bug quickly I can just do as you suggest

@georgettica
Copy link
Contributor Author

woot woot! tests are passing!

@georgettica
Copy link
Contributor Author

For me having this as json is a huge step forward, so any schema changes are welcome

@georgettica
Copy link
Contributor Author

tests have passed! feel free to review again @kellyjonbrazil

@georgettica
Copy link
Contributor Author

@kellyjonbrazil ?

@kellyjonbrazil
Copy link
Owner

Hi @georgettica - sorry I've been traveling quite a bit the past few months. I'll take a closer look soon and start working on wrapping up the next release.

@georgettica
Copy link
Contributor Author

No rush or problem, just thought it was lost between the cracks

Copy link
Owner

@kellyjonbrazil kellyjonbrazil left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Some items to address.

Copy link
Owner

Choose a reason for hiding this comment

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

Make sure to add the new parser to lib.py.

Comment on lines +7 to +9
Compatibility:

macOS
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need this section


macOS

Example:
Copy link
Owner

Choose a reason for hiding this comment

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

Missing a Schema section

return category, description


def parse_entry(line: str, headers: List[str]) -> Optional[Dict[str, Any]]:
Copy link
Owner

Choose a reason for hiding this comment

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

All of these function names should be preceeded by an underscore. (e.g. _parse_entry) Only parse should not start with an underscore. This will ensure the documentation is generated correctly.

@georgettica
Copy link
Contributor Author

Will address all of these issues soon.

Also, they all seem small, which is great

@georgettica
Copy link
Contributor Author

I think everything is ready to merge on my part, and PR is moving forward

@kellyjonbrazil
Copy link
Owner

I don't see the changes yet. Did you push them to the branch?

@georgettica
Copy link
Contributor Author

Yeah, I see there are no changes now, I'll test this soon and come vack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants