-
Notifications
You must be signed in to change notification settings - Fork 0
added dynamic parameters
for reuse and updated readme.md
#6
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Manas Malla <manasmalla.dev@gmail.com> Added input statements and applied processing to extract information for more dyanmicity and reusablility when generating scripts for multiple events and updated readme for more understanding on the params to change for specific events
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.
Shared my review comments. Please look into them.
1. Chrome |
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.
Need more info here. Could you please move the README changes to a separate PR so that we can work on it in length?
def __init__(self) -> None: | ||
print("Enter the name of the csv file") | ||
csv_file_path = input() |
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.
Would be preferable to get the path from the command line using the argparse module.
https://docs.python.org/3/library/argparse.html
This parser should be in the root of the project.
@@ -26,40 +26,40 @@ def get_base_dict(self): | |||
""" | |||
fetches the base template | |||
""" | |||
return self.load_json("templates/base.json") | |||
return self.load_json("../templates/base.json") |
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.
Using the prev directory specifier ../
depends on the current working directory.
If the user is executing from /
this will not work.
If the user is executing from 'src/` the will work.
Using an argparser and generating absolute paths during runtime should fix this.
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.
Updated it. --path and --csv have been added
2e6368c
to
3e9f063
Compare
Hi @ManasMalla, I don't see the requested changes in the PR. Were you able to push the changes? |
Signed-off-by: Manas Malla manasmalla.dev@gmail.com
Updated
README.md
for documenting the changes required in the files for every event.Updated the serializer and deserializer to take in dynamic inputs, like the csv file, and generate the respective .side file