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 regexp for better error handling in addauthor (orcide, inst, email) #238

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

jwbos
Copy link
Collaborator

@jwbos jwbos commented Mar 14, 2024

No description provided.

@jwbos jwbos requested a review from kmccurley March 14, 2024 17:51
@jwbos
Copy link
Collaborator Author

jwbos commented Mar 14, 2024

Do we need more checks or tests?

Copy link
Member

@kmccurley kmccurley left a comment

Choose a reason for hiding this comment

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

For now I'll leave this as comments.

}
\cs_new_eq:NN \IfValidInst \iacr_check_inst:nTF

\prg_new_conditional:Npnn \iacr_check_email:n #1 { T, F, TF }
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that we should check email addresses. Some people may like to put firstname dot lastname to obfuscate it, and I don't think we should interfere with that. The only purpose of the email address in the paper is to allow readers to contact at least one author, but we don't use it for any other registration (there is a separate validated email address from hotcrp when the corresponding author uploads their final version). It's also really complicated to recognize 100% of email addresses with a regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The point is not to check / validate the email address. We want to make sure this looks somewhat like a valid string used for an email address and not that a comma has been forgotten in the \addauthor macro.
IMO authors should not simply provide firstname dot lastname, if they provide an e-mail address it should resemble a real one.

\prg_new_conditional:Npnn \iacr_check_inst:n #1 { T, F, TF }
{
\cs_generate_variant:Nn \regex_match:nnTF {nV}
% An institute index should be a number, multiple institutes are separated by a comma:
Copy link
Member

Choose a reason for hiding this comment

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

Note that the numbers are supposed to start at 1 (not 0), but a later check will notice this when the paper is compiled. I don't think we need to check it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, same point as above. This is just a basic sanity check not a comprehensive validation.

@jwbos
Copy link
Collaborator Author

jwbos commented Mar 15, 2024

I will wait from merging until CiC Issue 1 has been published.

@jwbos jwbos self-assigned this Apr 16, 2024
@jwbos jwbos merged commit edb8dce into main Apr 16, 2024
1 check passed
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.

3 participants