Skip to content

chore: Ensure we're always using named indexes to access the internal data structure#784

Merged
krlmlr merged 14 commits intomainfrom
f-igraph-t-idx
Apr 21, 2023
Merged

chore: Ensure we're always using named indexes to access the internal data structure#784
krlmlr merged 14 commits intomainfrom
f-igraph-t-idx

Conversation

@krlmlr
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr commented Apr 18, 2023

This is a work in progress. Each commit should pass all tests.

Pre-work for #768 and #783.

@krlmlr krlmlr added this to the upgrade milestone Apr 18, 2023
@krlmlr krlmlr marked this pull request as draft April 18, 2023 15:37
## Drop all attributes, we don't want to deal with them, TODO
graph2 <- graph
graph2[[9]] <- list(c(1, 0, 1), list(), list(), list())
graph2[[igraph_t_idx_attr]] <- list(c(1, 0, 1), list(), list(), list())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krlmlr I think it should be replaced with some Call of C function. Should it be in this pull request?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Yes, please.

I wonder why the tests fail in CI/CD. Can you please try reverting the change to the .svg file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems slightly better to have all .Call() calls in separate R functions. So, this can be a call to a function whose only purpose is to .Call() a C function.

Copy link
Copy Markdown
Contributor

@Antonov548 Antonov548 Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why the tests fail in CI/CD. Can you please try reverting the change to the .svg file?

There is one failing test which is related to update old graph. I think it should be fixed after all properties will be fine. For now I just ignored it locally.

CC: test-old-data-type.R

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use skip() to officially skip the test for now, I had used that in this PR too.

@krlmlr krlmlr changed the title Draft: ensure we're always using named indexes to access the internal data structure chore: Ensure we're always using named indexes to access the internal data structure Apr 21, 2023
@krlmlr krlmlr marked this pull request as ready for review April 21, 2023 13:25
@krlmlr krlmlr merged commit 477e69c into main Apr 21, 2023
@krlmlr krlmlr deleted the f-igraph-t-idx branch April 21, 2023 13:25
@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Apr 21, 2023

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
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