Conversation
README.md
Outdated
|
|
||
| ```sh | ||
| git clone git@github.com:montesmariana/intro_machine_learning_using_python | ||
| git clone https://github.com/ElmoGeeraerts/intro_machine_learning_using_python |
There was a problem hiding this comment.
Well done but the url should end in .git
README.md
Outdated
|
|
||
| ```sh | ||
| python vendor_data.py <"-a/--add or -m/--modify"> | ||
| python vendor_data.py <-a/--add> |
There was a problem hiding this comment.
In this case and below, -a/--add and -m/--modify are actually literal, so you wouldn't need to add the <>.
README.md
Outdated
| - Look up specific vendors; | ||
| - Joining multiple excel files in the excel file created by this code. | ||
| - Join multiple excel files in the excel file created by this code. | ||
| - not create multiple entries for the same vendor if the excel file already exists and the ToExcel() method is called multiple times for the same vendor. |
There was a problem hiding this comment.
This item is not written very clearly, I'm confused as to what it means.
vendor_data.py
Outdated
| NewMail (str): a new e-mail address for a certain vendor | ||
| Function called: | ||
| CheckVendorMail(VendorMail) | ||
| Raises: |
There was a problem hiding this comment.
Check indentation in docstrings
vendor_data.py
Outdated
| How it works: | ||
| 1. The arguments provided are dumped into a dictionary called `Data`, | ||
| 2. A variable `FileName` is created, the name will be the value for `self.ProjectName` and `self.SourceLang` joined by an underscore (_). | ||
| 3. The dictionary `Data` is turned into a panda data frame. |
There was a problem hiding this comment.
pandas (you can also say pandas.DataFrame as technical name of the class).
vendor_data.py
Outdated
| if not os.path.exists(FileName): | ||
| df.to_excel(FileName, index=False, sheet_name="VendorData") | ||
| else: #if file exists, the data is appended to the existing file | ||
| return(f"The file {FileName} is correctly created and the vendor {self.VendorName} was added.") |
There was a problem hiding this comment.
Here and later, I would print() instead of return(). You are just providing a message, not an actual value.
When you return a value it's normally something that you want to do something with or that you can use to check that something went well. For example, you could return the dataframe itself or None, or True/False, so that someone can assign the value and see if things went well.
A message that is only meant to be seen, like a log, should be printed or logged (https://docs.python.org/3/library/logging.html). Printing is enough here, you can investigate logging for the future if you're interested (there the user can decide what kind of information gets printed and whether it's printed in the console/ notebook or to a file).
| df.to_excel(writer, sheet_name = "VendorData", index=False) #overwrite the existing excel file | ||
| df.to_excel(writer, sheet_name = "VendorData", index=False) | ||
| return("The vendor was correctly modified.") | ||
| else: |
There was a problem hiding this comment.
Very good!
You can also change the if os.path.exists('...') to if not os.path.exists('...'), throw the error of line 302 in that case, and then move on with the rest of the validation (without an else). That way you don't have so much indentation in lines 280 to 300.
But it's just an aesthetic thing, this is also perfectly fine.
vendor_data.py
Outdated
| print("Vendor was added!") | ||
| else: | ||
| print ("Action was cancelled.") | ||
| if args.modify: |
There was a problem hiding this comment.
Could someone run python vendor_data.py -a -m then?
There was a problem hiding this comment.
This isn't markdown anymore? It's a jupyter notebook with .md extension (not the same thing)
There was a problem hiding this comment.
And please, do not use absolute paths, since they only work for you! (e.g. in sys.append()) You can just append the directory where the tutorial lives by doing sys.path.append('../').
2 participants
No description provided.