Skip to content

Pass through the implementation of more index map operations. - #364

Merged
fitzgen merged 1 commit into
masterfrom
simplify-section-consumer
Oct 18, 2018
Merged

Pass through the implementation of more index map operations.#364
fitzgen merged 1 commit into
masterfrom
simplify-section-consumer

Conversation

@loganfsmyth

Copy link
Copy Markdown
Contributor

Indexed maps have a lot of duplication with basic maps right now, and I can't see a good reason for it, so this is an effort to normalize all of that logic on top of the basic map implementation.

This PR also includes support for computeColumnSpans on indexed maps, so that the two types of consumers have a consistent API again.

Beyond the expected stuff, this PR also includes one breaking change. The basic consumer's eachMapping function now returns lastGeneratedColumn as Infinity instead of null when you've called computeColumnSpans and there was no span end. Before this, it was null, but setting it to Infinity keeps the API consistent with generatedPositionFor and allGeneratedPositionsFor's lastColumn value.

@tromey

Copy link
Copy Markdown
Contributor

I don't really know much about this area so I think I will leave it to Nick.

@tromey
tromey removed their request for review October 16, 2018 19:44
@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 516

  • 53 of 55(96.36%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+4.2%) to 88.063%

Changes Missing CoverageCovered LinesChanged/Added Lines%
lib/source-map-consumer.js535596.36%
TotalsCoverage Status
Change from base Build 505:4.2%
Covered Lines:876
Relevant Lines:972

💛 - Coveralls

@fitzgenfitzgen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks @loganfsmyth :)

@fitzgen
fitzgen merged commit 7356bcb into masterOct 18, 2018
Sign up for freeto 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.

4 participants

@loganfsmyth@tromey@coveralls@fitzgen