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

fix: append text only works with Matchers #678

Merged
merged 1 commit into from Jun 3, 2021
Merged

fix: append text only works with Matchers #678

merged 1 commit into from Jun 3, 2021

Conversation

individual-it
Copy link
Contributor

@individual-it individual-it commented Jun 2, 2021

fixes #677

there were two problems in the code

  1. a typo in the variable name context vs content so the first if wasn't doing anything (introduced by b3b5e62#diff-254dbea027a5c57e2b14fb8ff30edc28fea1d39ff2392d95731f7a16f60d5782R70)
  2. the second else if compiled into the JS: else if (content), so all cases that had any content were send down that road

now the whole section compiles into

 if (typeof content === 'object' && content['pact:matcher:type']) {
     this.children.push(new xmlText_1.XmlText(content.value || '', content));
 }
 else {
     this.children.push(new xmlText_1.XmlText(content.toString()));
 }
 return this;

I deleted the fist if statement because it wasn't used in the first place and it should not harm to use toString() on a string

ToDo:

  • add tests

there were two problems in the code
1. a typo in the variable name `context` vs `content` so the first `if` wasn't doing anything (introduced by b3b5e62#diff-254dbea027a5c57e2b14fb8ff30edc28fea1d39ff2392d95731f7a16f60d5782R70)
2. the second `else if` compiled into the JS: `else if (content)` all cases that have any content set down that road

now the whole section compiles into
```
 if (typeof content === 'object' && content['pact:matcher:type']) {
     this.children.push(new xmlText_1.XmlText(content.value || '', content));
 }
 else {
     this.children.push(new xmlText_1.XmlText(content.toString()));
 }
 return this;
```

I deleted the fist if statement because it wasn't used in the first place and it should not harm to use `toString()` on a string
@individual-it individual-it marked this pull request as ready for review June 2, 2021 15:32
@individual-it
Copy link
Contributor Author

@TimothyJones this is ready for review

maybe the tests check too much private values, but I felt that it should be checked that the xml ends up being what it should.
Alternatively I could stringify the xml and then convert it to JSON, then all the eslint and ts exceptions would not be needed

@TimothyJones
Copy link
Contributor

First off, thanks for this! I appreciate all the time you've put in.

Alternatively I could stringify the xml and then convert it to JSON, then all the eslint and ts exceptions would not be needed

I think that stringification and conversion sounds like a nice approach, as long as it's not creating a lot of code in the test - I'd be concerned about bugs in a complex test harness - who tests the tests? However, as I said in slack, I'm not personally super familiar with patterns for handling xml in JS, so I'm not sure what is easiest.

In terms of review, I found the test a bit hard to follow - maybe it's just a style I'm not used to, but perhaps it would be more self documenting if we could break up the blocks of expect into more individual tests with more specific descriptions (even if they're still grouped together).

I'd prefer not to turn off the linter if we can avoid it. If it's necessary to get around the type system for testing, I feel like maybe the type system needs improvement, rather than the tests need to be allowed to break the rules.

That all said, this is an improvement and fixes a bug, so I'm inclined to merge it. If you'd like to rework the tests a bit, that would certainly be welcome. If you end up not being able to get to it, let me know and I'll take a crack.

@TimothyJones TimothyJones merged commit 5901021 into pact-foundation:feat/v3.0.0 Jun 3, 2021
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

2 participants