You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/02/25 17:43:41 UTC
[GitHub] couchdb-chttpd pull request: Return {error, Reason} for illegal_da...
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-chttpd/pull/26
Return {error, Reason} for illegal_database_name
COUCHDB-2625
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iilyak/couchdb-chttpd 2625-dbname_validator
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-chttpd/pull/26.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #26
----
commit 6d98bf5545adabdc9dd5fb21440fd5f341e94205
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-02-25T16:35:11Z
Return {error, Reason} for illegal_database_name
COUCHDB-2625
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Return {error, Reason} for illegal_da...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/26#issuecomment-85499451
Here is the reason for the change https://github.com/iilyak/couchdb-couch/commit/0e01a33cc6443f0690cc5f2460c0240b64eda335. However I did close the main PR. The rationale for changing message text was discussed in closed PR I linked above.
> @kxepal: In case of db name validation customization this error message shouldn't be defined here since the requirements for the name could be different.
Another orphan PR related to the issue: https://github.com/apache/couchdb-fabric/pull/14
The code duplication need to be eliminated anyway regardless of configurable dbname validator feature.
Also I do believe all errors need to be `{error, Reason}` across the system. Since it allows you to wrap functions when you need it without knowing all possible errors that wrapped function could return. You just handle `{error, Reason}` do error case logic and bubble `{error, Reason}` up.
I can create a new main PR which would just implement couch_db:validate_dbname and changes the error `{error, illegal_database_name, DbName}` -> `{error, {illegal_database_name, DbName}}`. I could create new jira issue with following description: `Remove code duplication in dbname validation`. If I go with second approach I would need to rewrite history for this PR to refer to different jira issue. I could create 2 different JIRA issues one for {error, Reason} and another one for code duplication.
Please advise what would be the better (I prefer second).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request #26: Return {error, Reason} for illegal_database...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak closed the pull request at:
https://github.com/apache/couchdb-chttpd/pull/26
---
[GitHub] couchdb-chttpd pull request: Return {error, Reason} for illegal_da...
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/26#issuecomment-85269588
The PR tile, linked ticket, and diff don't seem to match to me. The diff appears to change the error message to include the database name (which is fine, but I don't think we should change the text, just append "Provided name was 'foo'" or something).
It doesn't return {error, Reason} anywhere that I can see which the commit and PR title suggest this change does.
The linked ticket is about changing things to be able to configure a database name callback which doesn't have anything at all to do with this ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd issue #26: Return {error, Reason} for illegal_database_name
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-chttpd/pull/26
stale PR
---