Skip to content

Disable Debug Assertion for Parsing NPM Person objects#722

Merged
mjbvz merged 13 commits intomicrosoft:masterfrom
mjbvz:npm-person-parse-fix
Mar 17, 2016
Merged

Disable Debug Assertion for Parsing NPM Person objects#722
mjbvz merged 13 commits intomicrosoft:masterfrom
mjbvz:npm-person-parse-fix

Conversation

@mjbvz
Copy link
Copy Markdown
Contributor

@mjbvz mjbvz commented Mar 2, 2016

Defect
While debugging, Npm Person ends up throwing thousands and thousands of exceptions because the Json data it is receiving from NPM is invalid according to its current check. It expects an object, but a person in a package.json may also be a string: https://docs.npmjs.com/files/package.json#people-fields-author-contributors

Fix

  • Moved the parsing logic for Person to a factory named CreateFromJsonSource. This should make it clear to callers what this function does.
  • For parsing, also accept just regular json strings as input. The new factory does not parse this field currently but uses it as the name field for now. Parsing may be added later.
  • Added some unit tests to pin the existing behavior and test the new string behavior.

A whitespace only change to InteractiveWindow.cs is included in this PR. Git automatically created this change and I cannot undo it without manually editing the .gitattributes file. I think we just need to commit this file again so that git is no longer confused about the line endings in this file.

mjbvz added 5 commits March 1, 2016 16:47
This class is just a simple structure and its constructors should not be doing all this parsing.
This gets hit literally thousands and thousands of times during debugging, making it worthless. Person can be a string too, so the assertion is not even accurate.
@msftclas
Copy link
Copy Markdown

msftclas commented Mar 2, 2016

Hi @mjbvz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Matt Bierner). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

Comment thread Nodejs/Product/Npm/SPI/Person.cs Outdated
if (group.Success) {
Name = group.Value;
if (@group.Success) {
name = @group.Value;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Weird... I was wondering about those names but thought it was intentional. Reshaper's extract messed something up here. Will undo this.

@@ -1,310 +1,310 @@
/* ****************************************************************************
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.

You should be able to remove this change now that #709 has been merged

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented Mar 8, 2016

Ok, I removed the string person regex stuff for now, addressed the other comments, and have synced with master to get rid of any line ending changes.

@mjbvz
Copy link
Copy Markdown
Contributor Author

mjbvz commented Mar 16, 2016

@mousetraps can you please take another quick look at this review. This currently makes debugging a pain if you ever accidentally trigger the npm download, plus the assertion is not even correct to being with.

@mousetraps
Copy link
Copy Markdown
Contributor

test categories still need to be updated but 👍 after that!

mjbvz added a commit that referenced this pull request Mar 17, 2016
Disable Debug Assertion for Parsing NPM Person objects
@mjbvz mjbvz merged commit 66acb1b into microsoft:master Mar 17, 2016
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.

3 participants