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

feat(index.js): Add range notation support #13

Closed
wants to merge 2 commits into from

Conversation

fabiancook
Copy link

Resolves issue #1

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e71543d on fabiancook:feature/cell-ranges into 0b8d147 on mike182uk:master.

@fabiancook
Copy link
Author

It should be noted that detectRange has been included so that no existing code is broken, someone may be catching the error created and performing a different action.

toA1Range and toR1C1Range have been included for people who want to specifically convert ranges.

If no type is passed to range then it will auto-detect, however the two cell references should be the same, this is only an artificial limitation and if you think we should allow different types then we can change that.

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling ee39e31 on fabiancook:feature/cell-ranges into 0b8d147 on mike182uk:master.

@mike182uk
Copy link
Owner

Hey @fabiancook ,

Thanks for contributing this 😃

I've taken a look at your implementation and it looks to do the job which is great 👍

I think we can do a little refactoring though. I'am not keen on the the extra boolean param thats been added to most of the methods - i can see why you have done this, but i think it makes things a little more complicated. I am happy to bump the version to account for BC breaks if we need to.

On a sidenote this, is a good read on boolean params.

I really like your idea of passing a convertor to the convertRange method, nice way of reusing existing functionality.

Based on your changes, i think we could simplify things to something like the following:

module.exports.toA1Range = // ...
module.exports.toR1C1Range = // ...

var R1C1RANGE = // ...
var A1RANGE = // ...

function cellref(ref) {
  if (R1C1RANGE.test(ref)) {
    return convertRange(ref, convertR1C1toA1);
  }

  if (A1RANGE.test(ref)) {
    return convertRange(ref, convertA1toR1C1);
  }

  // ...
}

function convertRange(range, converter) {
    return range.split(':')
        .map(converter)
        .join(':');
}

function convertR1C1RangeToA1Range(range) {
    // check range is valid...

    return convertRange(range, convertR1C1ToA1);
}

function convertA1RangeToR1C1Range(range) {
    // check range is valid...

    return convertRange(range, convertR1C1ToA1);
}

This would then leave cellref to do the autodetecting and get rid of the boolean flags.

What do you think?

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

Successfully merging this pull request may close these issues.

None yet

3 participants