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 Google sheet capability (#10) fixes #9 #10

Merged
merged 3 commits into from
Jun 10, 2021
Merged

Add Google sheet capability (#10) fixes #9 #10

merged 3 commits into from
Jun 10, 2021

Conversation

RandallWert
Copy link
Contributor

Pull Request

I created a new Python program "csv_or_sheet_to_vanilla_i18.py" to add the capability of getting the csv data from a Google sheet.

I detect how many parameters were entered by the user. If three, then the second and third parameters are considered to be the SheetID and worksheet name, and the URL is built from them. In the main function, I look for the presence of "google.com" in the path; if it is found, this indicates that a sheet should be read via the URL. Otherwise, it is assumed that a local csv file should be read.

I tested again the csv and sheet that you provided, so running it against those should verify that it works.

This change creates the capability of reading a Google sheet, not just a local csv file.

  • [√] Major (I think?)
  • Minor
  • Patch

Description for the changelog

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

I tested and it works perfectly. Thanks @RandallWert . I would suggest some sanitation changes.

  • Please update the code in the existing python script instead of adding a new one.
  • Have default option for worksheet set as "Sheet1". When the user passes only a single param, check if it's a file path. Otherwise assume it to be sheet ID.
  • I don't think we should allow entry of URL since it may not be consistent. For instance, I believed a direct sheet link from the browser had to be put. I was wrong.
  • Update the readme with details on how the script works. The existing README.md inside csv_to_vanilla-i18n already contains some instructions.

@RandallWert
Copy link
Contributor Author

Hi, thanks for your feedback! I understand all of your points except the third one. I'm not sure exactly what I should change for that one. Should I just make a note in the readme, instructing the user not to enter an entire URL, but only the sheet ID (and possibly the worksheet name, if different from the default)?

@thealphadollar
Copy link
Owner

On the third point, I meant to remove the feature to add the URL as a specific URL pattern is required. Users may be more confused with that.

@RandallWert
Copy link
Contributor Author

OK, I'm trying to prevent that confusion by putting the URL pattern in the code, rather than asking the user to enter it. The user only has to be concerned with the sheet ID (and possibly the worksheet name, if different than the default).

I did my best to make the changes you requested to the Python script and the readme. I just pushed another commit with those changes.

Thank you again for the opportunity to work on this, and please let me know if I should refine the code any further.

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding exhaustive details to the README.

I've requested some more minor changes. Please push them and we can merge the changes.

csv_to_vanilla-i18n/README.md Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/README.md Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/README.md Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/README.md Outdated Show resolved Hide resolved
csv_to_vanilla-i18n/csv_or_sheet_to_vanilla_i18.py Outdated Show resolved Hide resolved
@RandallWert
Copy link
Contributor Author

OK, made those changes to the readme, and hopefully removed that unnecessary .py script successfully. (That's my first experience with that -- I'm still pretty new to making open source contributions.)

Copy link
Owner

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

LGTM. Your contribution is amazing considering it's your first time to open source. Looking forward to interacting more with you Randall.

@thealphadollar thealphadollar changed the title Add Google sheet capability Add Google sheet capability (#10) fixes #9 Jun 10, 2021
@thealphadollar thealphadollar merged commit 10f06c9 into thealphadollar:master Jun 10, 2021
@RandallWert RandallWert deleted the add-google-sheet-option branch June 10, 2021 06:06
@RandallWert
Copy link
Contributor Author

Thank you so much! It was a pleasure and helped me learn a lot! I appreciate the opportunity to contribute, and I look forward to more collaboration with you.

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.

2 participants