Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Fixed salti path resolution #11

Merged
merged 14 commits into from
Aug 9, 2016
Merged

Fixed salti path resolution #11

merged 14 commits into from
Aug 9, 2016

Conversation

andrew-aladev
Copy link
Contributor

@andrew-aladev andrew-aladev commented Aug 5, 2016

  1. Added schema check.
  2. Path resolution simplified.
  3. Removed irrational regexp from thali id.
  4. Both DB name and prefix has been cutted before actual path check. It shouldn't create any problem in future with any spec Write a spec on how to deal with the path prefix for DBs and Salti #9.

I think this closes previous problem with db prefix. Closes #10


This change is Reviewable

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling a0c3b1e on vNext_aaladev_10 into 8a5721b on master.

@yaronyg
Copy link
Member

yaronyg commented Aug 5, 2016

:lgtm:


Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


lib/index.js, line 237 [r1] (raw file):

    return false;
  }
  this.path = this.path.substring(this.dbPrefix.length);

NB: Personally I think that changing a state variable of a class as a side effect of a function isn't terribly discoverable. I would probably have designed this function to either return null if there is no path match or otherwise return the path and then set this.path higher up. You can put the whole thing into the if statement so you don't even need an extra line.


Comments from Reviewable

@yaronyg yaronyg assigned andrew-aladev and unassigned yaronyg Aug 5, 2016
@yaronyg
Copy link
Member

yaronyg commented Aug 8, 2016

This should be closed today

@andrew-aladev
Copy link
Contributor Author

Year, thank you. It looks perfect to rename cutDBPrefix to getPathWithoutPrefix and proceed the check with cutted path as local variable.

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1293fdb on vNext_aaladev_10 into 8a5721b on master.

@yaronyg
Copy link
Member

yaronyg commented Aug 8, 2016

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


lib/index.js, line 118 [r2] (raw file):

  // Next section deals with
  // /{:db}/{:id}, /{:db}/_local/{:id} and /{:db}/{:id}/attachment stuff.
  var cuttedPath = this.getPathWithoutPrefix();

NB:/cuttedPath/cutPath/


Comments from Reviewable

@yaronyg yaronyg assigned andrew-aladev and unassigned yaronyg Aug 8, 2016
@andrew-aladev andrew-aladev merged commit 2dadb44 into master Aug 9, 2016
@andrew-aladev
Copy link
Contributor Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the prefix option and related code form Salti
3 participants