Skip to content

refactor: make_empty_graph() is now fully auto-generated#1068

Merged
aviator-app[bot] merged 2 commits intomainfrom
refactor/make_empty_graph
Feb 8, 2024
Merged

refactor: make_empty_graph() is now fully auto-generated#1068
aviator-app[bot] merged 2 commits intomainfrom
refactor/make_empty_graph

Conversation

@szhorvat
Copy link
Copy Markdown
Member

@szhorvat szhorvat commented Jan 2, 2024

Related to #1140

make_empty_graph() is now fully auto-generated. The custom implementation is no longer needed after #1051.

This PR conflicts with the first commit from the draft PR #1044.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Jan 2, 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 PR was merged using Aviator.


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.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Jan 2, 2024

Why is this urgent?

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Jan 2, 2024

If you don't want to merge it now your can put it on hold. This was part of verifying that scalar conversion checks are working as intended and there are no crashes

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Jan 2, 2024

I should mention that this also catches errors that the previous version did not (passing a longer vector where a scalar is expected). I don't see any disadvantage to merging this, quite the contrary.

@szhorvat szhorvat mentioned this pull request Jan 3, 2024
41 tasks
@szhorvat
Copy link
Copy Markdown
Member Author

Is there any reason to delay merging this? Instead of saying it conflicts with the first commit in #1044, I should have said that it replaces that commit. This is not a breaking change, just simplifies codes and uses standard error reporting instead of a custom version.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Jan 20, 2024

We can merge right after the 2.0.0 release, hopefully next week, unless the revdepchecks show anything major.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 8, 2024

Do we never intend to expose or even use make_empty_attrs() ? Should we add a comment to explain the reasoning, like you did for other low-level functions earlier?

I still need to run revdepchecks on this.

…ty_attr()

igraph_empty_attr() is not needed for the R interface.
@szhorvat szhorvat force-pushed the refactor/make_empty_graph branch from 8930c11 to 43f14d3 Compare February 8, 2024 08:34
@szhorvat szhorvat force-pushed the refactor/make_empty_graph branch from 43f14d3 to 887a4f7 Compare February 8, 2024 08:36
@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 8, 2024

Do we never intend to expose or even use make_empty_attrs() ?

No

Should we add a comment to explain the reasoning, like you did for other low-level functions earlier?

Done

I still need to run revdepchecks on this.

Risk of breakage is very low, but always a good practice to run revdepchecks

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 8, 2024

Thanks!

@aviator-app aviator-app bot merged commit 8e08822 into main Feb 8, 2024
@aviator-app aviator-app bot deleted the refactor/make_empty_graph branch February 8, 2024 12:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants