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

Update with important fixes #18

Closed
MyRoGeertGit opened this issue Aug 25, 2024 · 4 comments
Closed

Update with important fixes #18

MyRoGeertGit opened this issue Aug 25, 2024 · 4 comments

Comments

@MyRoGeertGit
Copy link

Thank you all for this new effort.

In tfussell#748 (comment) @doomlar mentioned 3 important issues. As far as I can see, they're not included in this community release.
My attention has been drawn to it by issue tfussell@1eabb38.
I would appreciate them.

Geert.

@doomlaur
Copy link

I also discussed this in #9 with @m7913d. Personally, I had an issue with certain files that caused XLNT exceptions. So we would have 2 possibilities:

  1. Fix the exception I had by fixing issue Exception thrown when loading an XLSX file containing defined names without localSheetId tfussell/xlnt#685.
  2. Add full support for defined names (like Update definedNames tfussell/xlnt#686 does, but eventually with some further improvements).

Before we can decide what is the best way to go, I would have the following questions:

  • What is your use case, exactly?
  • What file(s) are you trying to load or create?
  • Would your issue be fixed by fixing certain exceptions (option 1), or do you need full support for defined names (option 2)?

@MyRoGeertGit
Copy link
Author

Our goal is to read spreadsheets files from xlsx and to write simple spreadsheets.
The spreadsheets are produced by other external platforms and contain typically database data. Important is that definedNames which users may be added aren't aborting reading of the spreadsheet, which is the case, if a name is defined on the workbook level. Making localSheetId optional is already sufficient for the moment. Your implementation tfussell#686 goes already much further, by implementing other optional keywords. Fixing the certain exceptions is sufficient. The following fix is what I needed.

@@ -2083,7 +2083,10 @@ void xlsx_consumer::read_office_document(const std::string &content_type) // CT_

                defined_name name;
                name.name = parser().attribute("name");
                {+if (parser().attribute_present("localSheetId"))+}
{+                {+}
                    name.sheet_id = parser().attribute<std::size_t>("localSheetId");
                {+}+}
                name.hidden = false;

@doomlaur
Copy link

The fix you are proposing makes sense. I will open a Pull Request tomorrow containing the fix. While I think that full support for definedNames might be desirable, for now it's important that XLNT does not throw unnecessary exceptions, so implementing your proposed fix is a step in the right direction 😉

doomlaur added a commit that referenced this issue Aug 29, 2024
@doomlaur doomlaur mentioned this issue Aug 29, 2024
doomlaur added a commit that referenced this issue Sep 1, 2024
Fixes issue #18 by making some attributes related to definedNames optional, as specified by ECMA-376.
@doomlaur
Copy link

doomlaur commented Sep 1, 2024

@MyRoGeertGit Your issue has been fixed by Pull Request #20. Please let us know if you find any other issues! 🙂

@doomlaur doomlaur closed this as completed Sep 1, 2024
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

No branches or pull requests

2 participants