-
Notifications
You must be signed in to change notification settings - Fork 997
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
Simplify output from make pyspec
#3938
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the clean up a lot!
left some comments but generally on board with these changes
@@ -394,8 +404,6 @@ def initialize_options(self): | |||
def finalize_options(self): | |||
"""Post-process options.""" | |||
if len(self.md_doc_paths) == 0: | |||
print("no paths were specified, using default markdown file paths for pyspec" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this PR change anything if someone does provide non-default paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I don't even know how to provide a non-default path. Is this something we do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if its used but it does seem like the code currently supports it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so it doesn't change any functionality with that. I just removed the print call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used the path flags, but Proto did when he designed it: https://github.com/ethereum/consensus-specs/tree/v1.5.0-alpha.6/tests/core/pyspec#advanced
Given that no current use cases require us to only build phase0 specs, I think it’s OK to ignore it for now.
# Ignore '1.5.0-alpha.*' to '1.5.0a*' messages. | ||
import warnings | ||
warnings.filterwarnings('ignore', message='Normalizing .* to .*') | ||
|
||
# Ignore 'running' and 'creating' messages | ||
import logging | ||
class PyspecFilter(logging.Filter): | ||
def filter(self, record): | ||
return not record.getMessage().startswith(('running ', 'creating ')) | ||
logging.getLogger().addFilter(PyspecFilter()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this stuff tends to be fragile so it could more easily break in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah maybe. Shouldn't be difficult to update if something breaks. Worst case we can just delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks refreshing. 😎
@@ -394,8 +404,6 @@ def initialize_options(self): | |||
def finalize_options(self): | |||
"""Post-process options.""" | |||
if len(self.md_doc_paths) == 0: | |||
print("no paths were specified, using default markdown file paths for pyspec" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used the path flags, but Proto did when he designed it: https://github.com/ethereum/consensus-specs/tree/v1.5.0-alpha.6/tests/core/pyspec#advanced
Given that no current use cases require us to only build phase0 specs, I think it’s OK to ignore it for now.
From this:
To this: