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

Correct writeXmlDoc.addMembers #184

Closed
wants to merge 3 commits into from
Closed

Correct writeXmlDoc.addMembers #184

wants to merge 3 commits into from

Conversation

ReedCopsey
Copy link
Contributor

Prevent members from being added to the global member list when the
xmlDoc contains no content.

Fixes #148

Prevent members from being added to the global member list when the
xmlDoc contains no content.
@latkin
Copy link
Contributor

latkin commented Jan 31, 2015

Can you add a quick test under tests\fsharpqa\Source\XmlDoc\Basic? Just to make sure garbage elements are no longer generated.

To check that no empty members are generated.
@ReedCopsey
Copy link
Contributor Author

@latkin Done. New xmlDoc005.fs test checks to make sure that names are generated for "Class1" but that there are no empty member names. This fails in current build, passes after this fix.

@latkin
Copy link
Contributor

latkin commented Jan 31, 2015

Some of the existing XMLDoc tests (under XmlDoc\UnitOfMeasure) are now failing due to the change. Can you take a look and see if the tests need updating to match new (correct) behavior, or if there is some regression?

@enricosada
Copy link
Contributor

👍 great

These previously required there to be an empty "member" variable for the
indices to match.  Adjusted indices to match new, correct generated XML
@ReedCopsey
Copy link
Contributor Author

@latkin Corrected those tests. Sorry I missed that the first round. This update should correct those tests to match the new, better generated content.

In general, these tests are fairly brittle. They're all based on specific ordering of the XML (which, actually, is kind of backwards now, though fixing would break most of the tests again.) It might make sense, in the longer run, to revisit how they're setup and run.

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