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

ReadSpreadsheet improvements #64

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

viroulep
Copy link
Contributor

Following #62 I ran into small details that I think deserve some fix/improvements:

  • google-auth-library is a peer dependency which was not pined; I believe the version must have changed a bit since you worked on this, and the way to authenticate has changed a bit. I've pinned the library dependency and made the necessary fixes (the json credentials to provide stay exactly the same though!).
  • I made parse a tiny bit more resilient to silly mistakes, such as trailing whitespaces, or lists with white spaces such as 2x2, 3x3 instead of 2x2,3x3.
  • I made an offset parameter: for Euros I create a "view filter" on the raw data and therefore I don't have an extra row to ignore; generally I think it should be a reasonable change.

}

get(row) {
return this.parse(row[this.value])
return this.parse(row.get(this.value))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This I could not explain, but row[headerValue] would return undefined; and the documentation here mentions .get, which does work in this case.
Not sure why it worked before, but do you see any reason to stick to []?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea. If this works then great :)

name: 'offset',
type: 'Number',
docs: 'Skip the first `offset` rows of the spreadsheet.',
defaultValue: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does change the existing behavior!
If this is not fine, I can definitely set 1 as the default value and adjust my code :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this is a better default, thanks! I'll set this in my scripts this year.

if (!identifierVal) {
return
}
identifierVal = identifierVal instanceof String ? identifierVal.toUpperCase() : identifierVal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to use this "trick" to deal with non-string identifiers (ie: wcaUserId).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, good call. I wasn't using non-string identifiers when I did this (only email, wcaId, name).

Copy link
Collaborator

@timreyn timreyn left a comment

Choose a reason for hiding this comment

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

Thanks!

}

get(row) {
return this.parse(row[this.value])
return this.parse(row.get(this.value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea. If this works then great :)

if (!identifierVal) {
return
}
identifierVal = identifierVal instanceof String ? identifierVal.toUpperCase() : identifierVal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, good call. I wasn't using non-string identifiers when I did this (only email, wcaId, name).

name: 'offset',
type: 'Number',
docs: 'Skip the first `offset` rows of the spreadsheet.',
defaultValue: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this is a better default, thanks! I'll set this in my scripts this year.

@timreyn timreyn merged commit 284544e into cubingusa:main Apr 18, 2024
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