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

Revisit reg_scrub.val_ok #7

Open
briangerard opened this issue Aug 14, 2016 · 2 comments
Open

Revisit reg_scrub.val_ok #7

briangerard opened this issue Aug 14, 2016 · 2 comments

Comments

@briangerard
Copy link
Contributor

The reg_scrub.val_ok method is pretty limited at present. It should allow for accented characters, for instance.

It would probably be worthwhile to see what well-vetted libraries are out there rather than rolling our own. This version was enough to get us safely out of the gate, but we should look at something more robust.

briangerard referenced this issue Aug 14, 2016
Add reg_scrub.py, to cleanse user input prior to use.  Mostly checking
for invalid characters in the values, and malformed variable names right
now.

Rearrange a touch in preparation for next usage check (del being the
only action if requested).
@aschrab
Copy link
Member

aschrab commented Aug 14, 2016

It's not only the method that is a problem. The way that it's used also has problems.

A missing or invalid value will just be skipped when attempting to create the Member object. So invalid or missing values can be silently ignored and cause fields that come later in the __init__'s argument list to be misinterpreted as long as there are enough values to fill in all of the required fields.

I think checking for missing or invalid fields would be better handled in the Member class. There should be different rules about what's valid for different fields anyway.

I'd also move away from passing all of those fields as positional arguments to __init__, that makes for a really awkward interface. Either pass them as a dict, or create the object empty and then set the attributes.

@briangerard
Copy link
Contributor Author

Yeah, I was planning on moving towards a per-field validation model. Like I said, this was in the interest of speed. :-) I like your idea of moving the field validation into the Member class, though. That sounds really promising.

Likewise on simplifying the Member instantiation. That was feeling pretty clunky. I'll file a new issue to track that bit, though.

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

No branches or pull requests

2 participants