This repository has been archived by the owner on May 2, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If I understand it correctly, there is one sheet per locale?
Is it then necessary to loop through the sheets here, can't we just loop through the locales and try to add them (from the code it seems like it throws an error if it does not exist)?
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.
That's correct, take a look at the sheet here: https://docs.google.com/spreadsheets/d/1ILFfc1DX4ujMnLnf9UqhwQGM9Ke3s1cAWciy8VqMHZw/edit#gid=462835362
The reason I'm looping over locales is because that way we will create sheets for new locales automatically. There is no way in the API to retrieve the sheet names in the spreadsheet. This way we don't have to worry about keeping locales and sheets in sync (it will sort it self out).
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.
Didn't read your comment thoroughly enough. Yeah, maybe we can get away without that loop.
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.
Haha getting late... The reason we have to loop over those are because there is no way of retrieving a sheet by its name. Does that make sense? The
doc.addSheet
doesn't return the sheet if it doesn't exist, it only throws an exception. So we have to, at some point, look at all the sheets to check if they match the current locale we are looping over.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.
I see, I thought the for-loop ended after adding a sheet. A little hard to wrap my head around the logic, but as long as it works it is more than good enough.
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.
I agree. Just refactored it, better now?