-
Notifications
You must be signed in to change notification settings - Fork 500
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
feat: Add function to validate an identifier #534
Conversation
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.
the functionality here looks good. i do think the function name identifier
is a little vague, but i don't have any alternatives to suggest so i'm going to approve this as-is
the codebase calls this a |
The bin (which is user-exposed, unlike the codebase) calls it |
Looking a little deeper at this, the impetus was #349 which this PR doesn't address. The line we've been using on these PRs is that if they were ready to go we could land them, but if they needed more work we'd close them and hope that folks who wanted the feature would revisit the work. I'm gonna close this in light of #349 which is the real bug to be fixed here. |
@wraithgar i have a local branch ready to fix #349 once this lands :-/ |
Can you implement it without having to export a new function? |
Certainly I can, but then consumers won't have an easily accessible predicate function to help explain why they got |
As I understand the closing comment, this PR needs more work. I cannot see any requests to change anything though, except perhaps the naming. |
In rebasing old PRs the line was drawn at "is this ready" and since it didn't address the underlying issue and folks wanted to bikeshed the name it was deemed "not ready" and closed. If there were someone actively making a new PR and responding to feedback it would likely be landed. |
Cleanup and linting of #351
Closes #350
Authored by @boxcee