sourceContentFor(aSource, nullOnMissing=false) should NEVER return NULL, but throw an exception instead

[GerHobbelt]

when sourceContentFor(aSource, nullOnMissing=false) shouldn't this situation (see patch/diff) also throw an exception? In the other situation, where this API might also produce a NULL return, an exception is thrown instead, so this looks to me like the more consistent behaviour.

I.e. one flow path does return NULL, while the other choses NULL or exception, depending on this nullOrMissing parameter. Which seems odd, at least to me.

[coveralls]

Pull Request Test Coverage Report for Build 560

• 5 of 6 (83.33%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 88.28%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/source-map-consumer.js	2	3	66.67%

Totals
Change from base Build 550:	-0.1%
Covered Lines:	825
Relevant Lines:	913

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 560

• 5 of 6 (83.33%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.1%) to 88.28%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/source-map-consumer.js	2	3	66.67%

Totals
Change from base Build 550:	-0.1%
Covered Lines:	825
Relevant Lines:	913

---

💛 - Coveralls

[GerHobbelt]

simile of #391 😢 Should better check/document my merges.

[andrewnicols]

Heh. Patches are almost identical except for unit tests. Great minds think alike and all that ;)

[GerHobbelt]

Heh. Patches are almost identical except for unit tests. Great minds think alike and all that ;)

Nah. It's rather more embarrassing: I had merged your code in my own fork, went on debugging some other other stuff in this lib and then when that was completed successfully, I did a manual inspection diff with the mainline, producing this one as one of the important changes. While I had been agonizing over the bugs elsewhere, I had completely forgotten about this merge.

Only after filing the pullreq and going through the other pending pullreqs did I notice I had picked up your code. Hence the edited-in remark above:

😢 Should better check/document my merges.

The only remaining worth is the extra unit tests.

[andrewnicols]

Sorry to break it to you, but this diffset has fewer tests than my issue. You’ve removed the ones in my patchset. At least if I read the changes tab properly.

[GerHobbelt]

Did a double-check: yes, you are absolutely correct. 😓 Thought I had it the other way around.

Killing this one, as it's a copy of #391 and y'all should ride with that one.

[GerHobbelt]

BTW, @andrewnicols : thanks for pointing this out; I had it completely wrong in my mind.

[GerHobbelt]

Dupe of #391