Skip to content

chore: Add checks to generated code #1577

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: Add checks to generated code #1577

wants to merge 1 commit into from

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Nov 7, 2024

From #1567.

Copy link
Contributor

aviator-app bot commented Nov 7, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

Copy link
Contributor

aviator-app bot commented Nov 7, 2024

This pull request failed to merge: some required checks failed. After you have resolved the problem, you should remove the blocked pull request label from this PR and then try to re-queue the PR. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed checks: pkgdown

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 7, 2024

@szhorvat: Have you seen this error before? Do you think this can be patched in a minimal way?

REAL() can only be applied to a 'numeric', not a 'NULL'

https://github.com/igraph/rigraph/actions/runs/11724389703/job/32658211635?pr=1577#step:7:41

@szhorvat
Copy link
Member

szhorvat commented Nov 7, 2024

This PR is broken out of #1567. The tests of #1567 pass. Why don't you just merge #1567 instead of trying to separate this out?

As I remember, removing the is.null checks is only valid if OPTIONAL foo was used, but not if foo=NULL was used. With OPTIONAL, Stimulus will generate the is.null checks by itself.

@krlmlr krlmlr removed the blocked label Nov 8, 2024
@krlmlr krlmlr force-pushed the f-stimulus-checks branch from 1678b3a to a49bb8b Compare November 8, 2024 07:19
@aviator-app aviator-app bot added the blocked label Nov 8, 2024
Copy link
Contributor

aviator-app bot commented Nov 8, 2024

This pull request failed to merge: some required checks failed. After you have resolved the problem, you should remove the blocked pull request label from this PR and then try to re-queue the PR. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed checks: pkgdown, Build source R package

@krlmlr
Copy link
Contributor Author

krlmlr commented Nov 8, 2024

Merging smaller PRs simplifies the review and gives greater confidence. I think we want to strive for a looser coupling between the C library and the R package to allow for more independent development of the two.

@szhorvat
Copy link
Member

szhorvat commented Nov 8, 2024

These changes, which clean up the interface, and try to better follow intended semantics (e.g. OPTIONAL vs =NULL) is precisely one step in the direction of more decoupling. But there's still a long way to go to get there. There are a lot of Risms in the interface definition file on the C side that we need to get rid of.

@szhorvat
Copy link
Member

szhorvat commented Nov 8, 2024

I don't think it's a good use of time, but if you want to decouple this into pieces, here's what I think can be done:

  • Adding all the IGRAPH_R_CHECK is independent of the interface update and can come first
  • Next comes updating the C core, which brings all the OPTIONAL instead of =NULL. This will cause a lot of duplicated/nested is.null checks.
  • Finally, you can remove the is.null checks from the yaml file.

Doing it in this clean and nice order wasn't realistic for me, as I kept discovering more and more issues while working on the codebase.

@maelle
Copy link
Contributor

maelle commented Apr 9, 2025

Only not draft PR without the hackathon label, so I'm putting it on the agenda 😸

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants