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

Ignore duplicate mappings #60

Merged
merged 10 commits into from
Apr 4, 2013

Conversation

usrbincc
Copy link
Contributor

@usrbincc usrbincc commented Apr 2, 2013

Two implementations.

  • The first one assumes that anything with the same generated location is a duplicate.
  • The other one does a full comparison.

I personally prefer the simpler implementation, if you can rely upon the mentioned invariant. But then again, who knows what kind of crazy corner cases may show up in the wild.

Or should differing mappings with identical generated locations be errors?

I tried to follow the prevailing coding style, but I couldn't resist giving strcmp its rightful name.

This should fix #21, assuming the added test is sufficient.

? mappingA.generated.column - mappingB.generated.column
: cmp;
return cmpLocation(mappingA.generated, mappingB.generated) ||
cmpLocation(mappingA.original, mappingB.original) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This throws if original is null.

Here is a test sokra@de77eb1

https://travis-ci.org/sokra/source-map/jobs/6008204#L57

@usrbincc
Copy link
Contributor Author

usrbincc commented Apr 3, 2013

Thanks for the null catch. I forgot about that possibility.

@usrbincc
Copy link
Contributor Author

usrbincc commented Apr 3, 2013

Question: are these considered duplicate mappings?

// Example 1
map.addMapping({
  generated: { line: 1, column: 0 },
  original: { line: 1, column: 0 },
  source: 'a.js'
});

map.addMapping({
  generated: { line: 1, column: 0 }
});

// Example 2
map.addMapping({
  generated: { line: 1, column: 0 },
  original: { line: 1, column: 0 },
  source: 'a.js',
  name: 'a'
});

map.addMapping({
  generated: { line: 1, column: 0 },
  original: { line: 1, column: 0 },
  source: 'a.js'
});

@fitzgen
Copy link
Contributor

fitzgen commented Apr 3, 2013

I don't have time to do an in depth review right now, but at first glance this looks pretty good.

Two comments now:

  1. I'd like tests that are a little more straight forward. Just create two maps, one where you only add mapping A, and the other where you add mapping A twice, and then just ensure that they are the same map. Maybe do this a couple times: with null mappings that don't specify an original position, with mappings that specify an original position but no name, and with mappings that specify all data possible.
  2. Is there a reason why we can't name the sort comparator, and then instead of doing the big if conditional on line 302, reuse that comparator function and just check to see if the result is 0?

Question: are these considered duplicate mappings?

I would say technically no, and handling that is outside of the scope of this pull request. However it is worth discussing this, so I have created an issue where both of your opinions are very welcome: #61

@usrbincc
Copy link
Contributor Author

usrbincc commented Apr 4, 2013

I'm glad I don't have to do the fuzzy matching from my question previously. It makes things much simpler.

I'm putting together a less fiddly test right now, but I thought I'd push all the non-test commits first.


Is there a reason why we can't name the sort comparator, and then
instead of doing the big if conditional on line 302, reuse that
comparator function and just check to see if the result is 0?

It was three things.

  1. Path dependency (check out the initial commit in this pull) -- very slippery slope.
  2. Premature optimization. Sorry, Donald (okay, so we're not really on a first-name basis).
  3. I was under the mistaken impression that previousOriginalLine and so on, were fill-ins for missing values. Upon closer inspection, I can see that this is for delta compression only, and missing values are just missing values.

In short, doh! Fixed in usrbincc/source-map@e834365

@fitzgen fitzgen merged commit cef697a into mozilla:master Apr 4, 2013
@fitzgen
Copy link
Contributor

fitzgen commented Apr 4, 2013

Thanks!

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.

Ignore duplicate mappings in SourceMapGenerator.prototype.addMapping
3 participants