Conversation
ZibanPirate
left a comment
ZibanPirate
left a comment
There was a problem hiding this comment.
awesome work @gorimaaa ! and you even went ahead and published the package 🚀
i added couple of comments, feel free to merge this PR now and address them in a separate follow-up PR if you want
thanks again for the PR i'm learning Python along with you now 🙏
There was a problem hiding this comment.
do we need to commit this file (and also kulya_python-1.6-py3-none-any.whl) ?
ideally we want to generate them and publish them in a Github CI Action
if they are not mandatory to be committed, then let's remove them for now, and worry about publishing the package in a follow-up PR.
| Issues = "https://github.com/dzcode-io/kuliya/issues" | ||
|
|
||
| [tool.hatch.build.targets.wheel] | ||
| only-include = ["src/kulya_python", "data"] |
There was a problem hiding this comment.
the "data" folder here is only used during build time right? and not actually published as part of the package code?
There was a problem hiding this comment.
can we also not commit this file? and generate it on the fly in CI? and also locally when you build the package
| self.nameAr = nameAr | ||
| self.nameEn = nameEn | ||
| self.nameFr = nameFr | ||
| self.univ = univ |
There was a problem hiding this comment.
why do we have .univ field?
| def getNameFr(self): | ||
| return self.nameFr | ||
|
|
||
| def getUniv(self): |
There was a problem hiding this comment.
why do we have getUniv method?
There was a problem hiding this comment.
can we not commit the __pycache__ folder?
| Essientially, it converts the JSON data into a Node object. | ||
| """ | ||
| def getNodeByPath(path: str) -> Node: | ||
| if DEVELOPMENT_MODE: |
There was a problem hiding this comment.
this is clever, but generally we want to avoid bloating our package with files it does not need. for instance, a consumer of this package will never use directoryToOneJsonFile but they will still download this piece of code regardless, which is not ideal.
an alternative solution is, move the directoryToOneJsonFile to a seperate non-published directory (eg: src/build/utils.py) and then:
- in development, you would need to run this in watch mode (it watches
../datafor changes and re-run, maybe usingnodemon) - in CI, we run this once before publishing the package (to prepare the json once)
think of it more or less like the JS implementation where we have a script that prepares the data.json
| targetFile.write(file.read()) | ||
| targetFile.write(",") | ||
| file.close() | ||
| targetFile.write('"":{}') |
There was a problem hiding this comment.
writing the json format by hand is error prune, maybe we can use a more rebust solution by using the existing json module, where:
- we would have a local jsonData variable as an empty array, that we then add nodes into
- then we json.dump it to targetFile
this way we guarantee we have a valid and escaped json file, and as a bonus we have a nice formatted file ;)
2 participants
Description
Following #109 #109
I added the modifications as asked :
If there is any error left, or misunderstanding from my side please let me know.
Type of change
Please delete options that are not relevant.