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

refactor: spec_finder.py #89

Closed
wants to merge 10 commits into from

Conversation

prnam
Copy link
Contributor

@prnam prnam commented Sep 21, 2024

This PR

  • applies ruff format
  • adds missing docstrings for main
  • uses %s formatting in logging functions
  • adds encoding
  • refactors extract_spec_map & parse_rust_files code from main
  • refactors get_spec function
  • adds logging config
  • adds docstrings for _demarkdown & get_spec function
  • improves message sentence

Notes

Also, fixes parsing error for the specification.json

@gruebel
Copy link
Member

gruebel commented Sep 21, 2024

hey @prnam thanks for the contribution, but there are too many unrelated changes in one PR, which makes it tedious to review. For example, don't apply a radical formatting, which changes mostly single quotes strings to double quotes. This is just a helper script, not really sure it needs proper logging or docstrings. Please create separate PRs for a subset of changes, then it can be reviewed better.

@prnam
Copy link
Contributor Author

prnam commented Sep 22, 2024

hey @prnam thanks for the contribution, but there are too many unrelated changes in one PR, which makes it tedious to review. For example, don't apply a radical formatting, which changes mostly single quotes strings to double quotes. This is just a helper script, not really sure it needs proper logging or docstrings. Please create separate PRs for a subset of changes, then it can be reviewed better.

Hey @gruebel
Could you please go through each commit one at a time?

This will help you understand how each change builds on the previous one and how it relates to the refactoring of this Python file.

@beeme1mr
Copy link
Member

@justinabrahms you may be interested in this one 😄

@prnam
Copy link
Contributor Author

prnam commented Sep 22, 2024

@justinabrahms you may be interested in this one 😄

If it still requires separate PR, then let me know which should be separate ones among the commits, so that I can raise it separately with approved list of type prefix. Thanks in advance. 🙂

Copy link
Member

@justinabrahms justinabrahms left a comment

Choose a reason for hiding this comment

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

I like a lot of the changes in here. Unfortunately, I'm hoping that we can delete the spec_finder.py repo entirely in favor of a docker image that's used across languages. See #73. The new specfinder code is at https://github.com/open-feature/spec/tree/main/tools/specification_parser


if missing:
logging.info('In the spec, but not in our tests:')
for m in sorted(missing):
logging.info(f" {m}: {spec_map[m]}")
logging.info("%s: %s", m, spec_map[m])
Copy link
Member

Choose a reason for hiding this comment

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

Just a note that this lost it's preceeding whitespace, which my fuzzy memory tells me wsa to help with visual parsing of the output.

spec_finder.py Outdated
@@ -1,11 +1,15 @@
#!/usr/bin/env python
import logging
Copy link
Member

Choose a reason for hiding this comment

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

I would personally not introduce the logging module. It's too heavy weight for CLI output, imo.



if number in missing:
missing.remove(number)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this in the new code.

@prnam prnam closed this Oct 1, 2024
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