Merge of "Improve OBConversion::WriteString() and WriteFile() initialization" (adalke) #1923 #2176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… otherwise the formats won't initialize correctly. (Issue openbabel#1922) This commit includes a new test program which checks WriteString() and WriteFile() for most of the supported formats. Without this patch, several of them will fail, and the FPS format will segfault.
…iple records. My first patch set Index=1 in WriteFile() and WriteString() so that the output format plugin could perform any initialization. This worked, but there was still a problem with some formats when saving multiple records, including the CML format. The CML format uses <molecule> as the top-level element when there is only one input molecule, and <cml> as the top-level element when there are two or more molecules. The <cml> element contains a sequence of <molecule> elements. If Index==1 and IsLast()==true then it's a single-molecule file. If Index==1 and IsLast()==false then it's a multi-molecule file. However, OBConversion::Write() always called SetOneObjectOnly(), which set Last=true, so the CML format always thought it was writing a single-record output file. The fix was to remove SetOneObjectOnly() from the Write() and do more intialization in WriteFile() and WriteString(). Previously Write() did Index++ *after* calling the format's WriteMolecule(). This was incorrect. It needs to be done *before*, so the value of GetOutputIndex() matches the number of molecules which were sent as output. (This is a better emulation of how Convert() works.) This in turn means WriteFile() and WriteString() need to reset Index to 0. They then call Write(), which increments Index, so WriteMolecule() always sees Index=1 for the first molecule.
…nt changes to) the SVG writer to be consistent with the rest.
… anyone reading this: please avoid random number generators at all costs.)
|
Aha - now it's failing in the tests. So I need to make @fredrikw's change after all. :-) |
… recommended by codacity code review.
|
Just to check before I review later today - are there any API changes? |
|
After checking, I don't think so. There are only a few code changes. Is there any github action for api changes? |
|
The main question is whether headers are changed. I'll see if I can create an action using some of the existing tools, but for now it's human checks. 😄 |
|
Looks like a good improvement, thanks for the work to both @adalke and @baoilleach |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See discussion over at #1923. This is a merge of work by @adalke.
I needed to rebase, hence the separate PR. I also needed to update some of the tests for changes in the interim.
I've made an attempt to get the tests working on Windows (e.g. normalizing line endings), but I think that some of the tests genuinely fail on that platform, e.g. the binary file handling, but presumably these never worked in the first place.
I haven't made the change described by @fredrikw over at #1923. That test passes for me, so I've avoided making the change - I'm not sure why it's passing now.