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

Add hook for the pysnmp_mibs package #34

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JoshDavisChem
Copy link

Hook for the pynsmp_mibs package at
https://pypi.org/project/pysnmp-mibs/

It is a package that contains a wide variety of MIB's precompiled for the pysnmp package

@JoshDavisChem JoshDavisChem requested review from a team and bwoodsend and removed request for a team August 18, 2020 13:04
Added comment description
New edits for white space issues
@bwoodsend bwoodsend mentioned this pull request Aug 18, 2020
@bwoodsend
Copy link
Member

@JoshDavisChem For the benefit of those of us who've never heard of this package or MIBs before (like me) could you give a brief overview of what the issue with this package is (without this hook). It appears to be pure Python which usually means no issues.

I'm mostly interested in how these xxx-MIB.py files are loaded? I take it that it is done by filename rather than Python's native from pysnmp_mibs import xxx? This would explain why you're treating python files as data files instead of hidden imports.

@bwoodsend
Copy link
Member

I should also say... thanks for working on this.

Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

The actual code looks good. Just need some explanation. Preferably in the comments of the hook file itself.

news/34.new.rst Outdated Show resolved Hide resolved
JoshDavisChem and others added 2 commits August 20, 2020 13:08
Adjust language for present tense.

Co-authored-by: Brénainn Woodsend <[email protected]>
@JoshDavisChem
Copy link
Author

This is still on my radar. I just have to go back to my original program and other documentation to review my comments and make them more specific.

@bwoodsend
Copy link
Member

OK, no rush - we're not going anywhere...

@Legorooj
Copy link
Member

Legorooj commented Jan 1, 2021

Any update on this?

@JoshDavisChem
Copy link
Author

JoshDavisChem commented Jan 5, 2021 via email

@Legorooj Legorooj force-pushed the master branch 10 times, most recently from 8003b05 to df3371d Compare June 26, 2021 05:59
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.

3 participants