You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by benkeen <gi...@git.apache.org> on 2014/11/22 02:00:14 UTC
[GitHub] couchdb-fauxton pull request: Move Helpers.docs into constants.js
GitHub user benkeen opened a pull request:
https://github.com/apache/couchdb-fauxton/pull/164
Move Helpers.docs into constants.js
This is a house cleaning ticket. It does a couple of things:
1. Moves the list of URLs stored in Helpers.docs into constants.js
2. Refactors constants to group the (now growing) list into appropriate
groups.
Now there are a lot less hardcoded "magic" strings throughout our
code, yay!
Closes COUCHDB-2474
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/benkeen/couchdb-fauxton 2474-helper-docs-moved-to-constants
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-fauxton/pull/164.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 #164
----
commit 7db88f6cb806bae941393526fefbfe8ae68dd311
Author: Benjamin Keen <be...@gmail.com>
Date: 2014-11-22T00:57:15Z
Move Helpers.docs into constants.js
This is a house cleaning ticket. It does a couple of things:
1. Moves the list of URLs stored in Helpers.docs into constants.js
2. Refactors constants to group the (now growing) list into appropriate
groups.
Now there are a lot less hardcoded "magic" strings throughout our
code, yay!
Closes COUCHDB-2474
----
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20796698
--- Diff: app/addons/fauxton/components.js ---
@@ -239,7 +239,7 @@ function(app, FauxtonAPI, ace, spin, ZeroClipboard) {
hideTray: function () {
var $tray = this.$('.tray');
- $tray.velocity('reverse', FauxtonAPI.constants.TRAY_TOGGLE_SPEED, function () {
+ $tray.velocity('reverse', FauxtonAPI.constants.MISC.TRAY_TOGGLE_SPEED, function () {
--- End diff --
A bit of a nit pick. These fixes are unrelated to the docs helper. Try and avoid sneaking fixes that are unrelated to the PR.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen closed the pull request at:
https://github.com/apache/couchdb-fauxton/pull/164
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20801689
--- Diff: app/addons/databases/resources.js ---
@@ -31,8 +31,8 @@ function(app, FauxtonAPI, Documents) {
});
},
- documentation: function(){
- return "all_dbs";
--- End diff --
Ok, you make a valid point. Lets leave the FauxtonAPI.constants in then. Returning just the `"all_docs"` does add a lot of magic which is probably unnecessary.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20796879
--- Diff: app/addons/databases/resources.js ---
@@ -31,8 +31,8 @@ function(app, FauxtonAPI, Documents) {
});
},
- documentation: function(){
- return "all_dbs";
--- End diff --
What about keeping the `"all_dbs"` and then in the api url it matches that text string to a `constant.DOC_URLS`. Maybe just loop through the DOCS_URLS and match the key.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#issuecomment-64226930
Any other feedback on this ticket anyone, or am I good to merge?
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20801583
--- Diff: app/addons/fauxton/components.js ---
@@ -239,7 +239,7 @@ function(app, FauxtonAPI, ace, spin, ZeroClipboard) {
hideTray: function () {
var $tray = this.$('.tray');
- $tray.velocity('reverse', FauxtonAPI.constants.TRAY_TOGGLE_SPEED, function () {
+ $tray.velocity('reverse', FauxtonAPI.constants.MISC.TRAY_TOGGLE_SPEED, function () {
--- End diff --
Oh ok. No prob. You caught me. I didn't read the commit msg just the heading.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#issuecomment-64061359
N.B. this is a pretty straightforward ticket, but it might be good to review as soon as possible due to the large number of files it touches.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#issuecomment-64450336
Merged as 07fe073
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20798134
--- Diff: app/addons/databases/resources.js ---
@@ -31,8 +31,8 @@ function(app, FauxtonAPI, Documents) {
});
},
- documentation: function(){
- return "all_dbs";
--- End diff --
Yeah that would totally work. This is such a subtle point, but I guess I'd argue that it's clearer with the long constants name. If the `documentation` method was `docUrl` or something to clearly indicate that we're returning a documentation *URL*, I'd be more in favour of returning the `all_dbs` string, because you could infer what it was just from looking at the code. However, you still wouldn't know where to find the URL. At least with the long constant it's immediately clear where the info's stored.
But I'm okay with your suggestion. So it would return the uppercase constant name?
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by garrensmith <gi...@git.apache.org>.
Github user garrensmith commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20780027
--- Diff: app/addons/databases/resources.js ---
@@ -31,8 +31,8 @@ function(app, FauxtonAPI, Documents) {
});
},
- documentation: function(){
- return "all_dbs";
--- End diff --
I like the idea of the docs being in `constants.js` but I don't like that we have to go from `"all_dbs"` to `FauxtonAPI.constants.DOC_URLS.ALL_DBS` everytime we want a documentation string. Could we not have a way of mapping the doc strings so that we don't have to do that?
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by robertkowalski <gi...@git.apache.org>.
Github user robertkowalski commented on the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#issuecomment-64429705
+1
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20795268
--- Diff: app/addons/databases/resources.js ---
@@ -31,8 +31,8 @@ function(app, FauxtonAPI, Documents) {
});
},
- documentation: function(){
- return "all_dbs";
--- End diff --
Yeah, this was my original concern with having to type out `FauxtonAPI.constants` instead of just `C` or something. It got verbose awfully fast.
I don't have another solution other than storing the var in a local var to reduce the length, or maybe renaming the main constants var to `FauxtonAPI.C`. Meh. :(
But personally I don't think this is all that bad. It's certainly verbose but clear.
---
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-fauxton pull request: Move Helpers.docs into constants.js
Posted by benkeen <gi...@git.apache.org>.
Github user benkeen commented on a diff in the pull request:
https://github.com/apache/couchdb-fauxton/pull/164#discussion_r20797470
--- Diff: app/addons/fauxton/components.js ---
@@ -239,7 +239,7 @@ function(app, FauxtonAPI, ace, spin, ZeroClipboard) {
hideTray: function () {
var $tray = this.$('.tray');
- $tray.velocity('reverse', FauxtonAPI.constants.TRAY_TOGGLE_SPEED, function () {
+ $tray.velocity('reverse', FauxtonAPI.constants.MISC.TRAY_TOGGLE_SPEED, function () {
--- End diff --
Oh! Actually this was part of the scope (see commit msg above) but the commit subject didn't include it & it should have, sorry about that.
---
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.
---