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 of the python support #109

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Add of the python support #109

merged 1 commit into from
Feb 12, 2025

Conversation

gorimaaa
Copy link
Contributor

@gorimaaa gorimaaa commented Jan 9, 2025

Description

Add of the python support, creating the API getNodeByPath that reads into the json file and create a node object that contains the data
Fixes #12

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Sorry, something went wrong.

Copy link
Member

@ZibanPirate ZibanPirate left a comment

Choose a reason for hiding this comment

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

nice, this is a great start

I have just a few comments before we merge the PR:

  • let's format the code
pip install black && black .
  • this code works great as a script, but we want to eventually publish it as a package in PyPi, for this to be possible we need to modify a few things (which can be done in a separate PRs later)
    • since the _data/ directory is outside the python/ directory, we should have a step that runs before publishing the package, where we essentially move _data/ inside python/ dir.
    • second, consuming the python/_data/ folder as is is not very efficient, thus, we need to modify the previous step, from just copying the _data/ folder over, to first parsing it and converting it into one single data.json file (similar to what we do for other languages)
    • following up on that, we need to modify our python code to just consume that data.json file, meaning that we don't have to pass absulote path in getNodeByPath(path: str) instead only the relative path of the node itself, eg:
      # from this
      getNodeByPath("path-to-_data-folder/umkb/fst/info.json")
      # to this
      getNodeByPath("umkb/fst")
    • finally we get rid of def main() and prepare the python/ folder to be shipped as a package following official docs

@gorimaaa
Copy link
Contributor Author

I see, I'll try to fix that and do another pr, thanks for your time

Copy link
Member

@ZibanPirate ZibanPirate left a comment

Choose a reason for hiding this comment

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

Let's go ahead and merge this PR for now, and we'll make improvements over time 🚀.

It's better to iterate on existing work rather than waiting for a perfect solution from the start, especially in open-source projects.

@ZibanPirate ZibanPirate merged commit 6ca5f1a into dzcode-io:main Feb 12, 2025
1 check failed
@gorimaaa
Copy link
Contributor Author

Great, I'll try to improve that as soon as possible

@gorimaaa gorimaaa mentioned this pull request Feb 17, 2025
2 tasks
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.

add Python support
2 participants