jmcnamara / libxlsxwriter

A C library for creating Excel XLSX files.
https://libxlsxwriter.github.io
Other
1.48k stars 330 forks source link

Add signature file for VBA projects (2nd try) #415

Closed HolgiHo closed 11 months ago

HolgiHo commented 11 months ago

PR for issue #411, 2nd try.

Library changes:

Tests:

Examples:

Utilities for testing the VBA project is signed correctly:

jmcnamara commented 11 months ago

Moving comments to new PR.

I think a better function name would be workbook_add_signed_vba_project_signature(lxw_workbook self, const char vba_project, const char *signature).

Do you mean workbook_add_signed_vba_project (without "signature)?

Yes. workbook_add_signed_vba_project(lxw_workbook *self, const char *vba_project, const char *signature)

HolgiHo commented 11 months ago

Hi, what about the falling jobs? Is there a misconfiguration of the container used for the workflows?

jmcnamara commented 11 months ago

Thanks. The latest changes look good.

Let's deal with the PRs one at a time so that there isn't overlapping emails:

  1. libxlsxwriter VBA code signing
  2. libxlsxwriter const *char issues
  3. libxlsxwriter ctest/unit test issues
  4. XlsxWriter VBA code signing

For this PR could you do the following:

Note, the Valgrind CI test is failing but I think that is due to some changes in the GitHub Actions backend. I'll look into that separately.

jmcnamara commented 11 months ago

Note, there is some trailing whitespace in places not covered by the indent check. Could you remove those from the commit.

jmcnamara commented 11 months ago

Hi, what about the falling jobs? Is there a misconfiguration of the container used for the workflows?

Don't worry about it. It is just something that has changed in the CI container. It was working last week and nothing in your PR would affect it. It happens periodically. I'll look into it separately.

HolgiHo commented 11 months ago

Note, there is some trailing whitespace in places not covered by the indent check. Could you remove those from the commit.

Could you provide a hint where you found them? Also including the locations in the indent check would be a good idea ;-)

jmcnamara commented 11 months ago

Could you provide a hint where you found them?

I just did a git show on the commit. I have the color.ui=auto option turned on so it shows trailing whitespace with a bright red square.

Also including the locations in the indent check would be a good idea ;-)

The trailing whitespaces are in the dox files and in the examples (which aren't checked/formatted by default to allow for some additional non-automatic formatting for instruction purposes).

I normally configure my editor(s) to remove trailing whitespace on save.

HolgiHo commented 11 months ago

Believe it or not, VS has no option to delete trailing spaces :-( There is a plugin, but it isn't compatible with VS 2022.

I know how to squash multiple commits into one in a git repo, but what I don't know is: Do I need a new PR for this? I think I even need to create a new branch on the remote (or delete and re-create it), right?

HolgiHo commented 11 months ago

Btw. if (!workbook->vba_project) isn't even necessary anymore in packager.c. workbook_add_signed_vba_project will never set workbook->vba_project_signature when workbook->vba_project is not set, so the test is redundant.

jmcnamara commented 11 months ago

Merged. Thanks.