Merge further pull requests

[doomlaur]

Now that a few important pull requests have been merged to our repository, I think it's time to discuss our strategy regarding merging further pull requests - and, in particular, pull requests that have not been authored by us. Due to the fact that we have merged our repository from the official XLNT repository, like we both agreed on, there are 2 pull requests that are critical for my use case before I can switch our software to this repository:

1. Update definedNames #686
2. Update pane struct to match spec #688

I'm particularly asking about those two now because I merged them a couple of months ago to my own repository and they worked very well for my use case. Unfortunately, without these, I cannot open certain files (due to XLNT throwing exceptions), so for now I still have to use my own fork.

Furthermore, there are further pull requests in the original repository that we might also think of merging. However, there's one disadvantage: by taking the pull requests directly, we cannot ask for further changes/improvements (unless we are lucky enough that the maintainer answers and also wants to cooperate with us).

[m7913d]

I'm not completely happy with the current state of those PRs:

1. Update definedNames tfussell/xlnt#686 exposes detail::defined_name in the public API. This seems to me a no-go. At least defined_name should be moved outside the detail namespace if we want to expose it in our API. Changing from a struct to a "real" class (i.e. hide member variable, expose content using function). This allows easier expanding if needed without breaking API backwards compat.

2. Update pane struct to match spec tfussell/xlnt#688 This changes a public interface. If we honor semantic versioning, we should switch to version 2.*. Note sure if that is the best approach for a new clone, as it may make it more difficult for others to switch to our repo. Not sure if we need to make all the fields optional, just comparing with it default value make is more space efficient.

Those remarks were the reason why I did not already created a PR and may have been some of the concerns why tfussell hadn't fixed it himself although he considered tfussell#388 as a high priority issue.

If you want those changes to be included, reaching out to musshorn may be a good starting point. If he isn't interested anymore, we may have a look at it ourselves. I don't know the exact reason why you need those changes:

• Update pane struct to match spec tfussell/xlnt#688 probably to just avoid a parsing error or are you interested in the actual value? Just converting the double to an int during parsing may be a simple temporary solution, before we change the interface.
• Update definedNames tfussell/xlnt#686 do you need to query the definedNames?

Depending on your needs and time you want to spend on it, we may also create our own solution that fixes your issues without all the extras.

I don't think there is a copyright issue if we either merge their commits (+ own changes) or create or own solution (if it is based on their implementation, we may mention it in the commit message and/or code). In fact by creating a PR to tfussell's repo, they already agreed with MIT license.

[m7913d]

Concerning the other PR / Issues: it is not my intention to have a look at them, unless I encounter the same issue or the poster also creates the issue or PR on our repo. Handling all the old items is just a lot of work, without a (significant) positive impact on the XLNT community.

[doomlaur]

I had issue tfussell#685, which has also been reported in tfussell#711, an issue which was fixed by pull request tfussell#686. I cannot remember if I actually had the issue fixed by pull request tfussell#688, but it improved conformance to the standard, which was what I wanted at the moment.

Background of this: I had multiple cases over time where XLNT couldn't read files that were correct and could be opened without issues by Excel, because XLNT did not always correctly conform to the standard - one of them being #1. A few of the pull requests have been already approved by tfussel some time ago, and for some others I was lucky enough to find some pull requests that fixed them. I must admit that it was at a time where I had no time to actually check the quality of the PRs (not ideal, I know), and due to the fact that I never meant my fork to be the next major XLNT fork, I simply merged them because they fixed some of my issues, without further thought.

Now, you are fully right - we want our fork to contain high-quality code (and I really enjoy our collaboration 😄), so it makes sense to take a very close look at what we merge. However, I personally do not need support for definedNames, but rather I want XLNT to open the file without issues. I can, however, gladly take a look again at the issue and fix it 😉

[m7913d]

The issue with those PR is not really the "quality" of their code (I would be fine to merge it in my personal version of XLNT if needed), but the (future) backwards compatibility, which isn't that important for a personal project. So feel free to base your solution on their code.