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

Mark *_wrap.cxx as generated #134

Merged
merged 1 commit into from
Apr 6, 2024
Merged

Conversation

robinst
Copy link
Owner

@robinst robinst commented Apr 6, 2024

See https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github

This will cause GitHub to collapse the changes in diffs by default. The changes can still be viewed by expanding the diff if desired. Should make it a bit easier to review PRs with changes to wrappers.

See https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github

This will cause GitHub to collapse the changes in diffs by default.
The changes can still be viewed by expanding the diff if desired.
Should make it a bit easier to review PRs with changes to wrappers.
@robinst robinst requested a review from jacobvosmaer April 6, 2024 07:45
@jacobvosmaer
Copy link
Collaborator

Makes sense!

Quasi-related: do we have some sort of CI check that the wrap.cxx files match the .i's?

@jacobvosmaer jacobvosmaer merged commit 33066c1 into main Apr 6, 2024
0 of 4 checks passed
@robinst
Copy link
Owner Author

robinst commented Apr 6, 2024

Quasi-related: do we have some sort of CI check that the wrap.cxx files match the .i's?

No, but that would be a good idea. (Are you thinking about the xz incident?)

@jacobvosmaer
Copy link
Collaborator

There is the xz angle but I think it's also just a normal mistake one can make when checking generated content into source control.

When I try to work on taglib-ruby it happens all the time I run swig against a version of taglib that is too new or something and it generates lots of wrapper code I don't want. It makes the code base hard to work with.

@jacobvosmaer
Copy link
Collaborator

jacobvosmaer commented Apr 6, 2024

I'll make an issue for this.

#136

@robinst robinst deleted the robinst-mark-generated-code branch April 6, 2024 16:08
@jacobvosmaer
Copy link
Collaborator

@robinst I'm starting to wonder if we should revert this change. Changes to the wrap.cxx files are relevant and we should review them. In #137 I discover that I find it annoying that I have to click to load each wrap.cxx diff.

@robinst
Copy link
Owner Author

robinst commented Apr 7, 2024

Hm yeah, I see. I wish there was a button to load all diffs. You could chuck this into the Console to do that:

document.querySelectorAll('.load-diff-button').forEach(node => node.click())

Why I still like them collapsed by default is: I usually want to review .i changes first, before the generated files. Maybe I should use the "Filter changed files" a bit more for that instead. But yeah, happy to revert, feel free to raise a revert PR.

@jacobvosmaer
Copy link
Collaborator

You could chuck this into the Console to do that:

Thanks but I'm never going to remember that. :)

Revert PR: #138

@jacobvosmaer
Copy link
Collaborator

I think auto-collapsing really only makes sense if you can trust the generated data is correct (because CI checks it) and it's not something you want to review.

Even though the wrap files are a huge verbose mess, I find it very important to review them because they reveal if Swig did the job you wanted it to do.

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.

2 participants