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

fix!: Clean error message when trying to access a graph that has been saved with a newer or an older version of the package, graph_version() returns an object of class "package_version" #832

Merged
merged 4 commits into from
Jun 10, 2023

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Jun 5, 2023

Closes #830

@szhorvat
Copy link
Member

szhorvat commented Jun 5, 2023

This changes the type of the object returned by the public and documented graph_version() function. Moreover, the kind of object returned compares differently than the strings that were previously used:

> as.package_version('0.10.0') < as.package_version('0.8.0')
[1] FALSE
> '0.10.0' < '0.8.0'
[1] TRUE

So this is a breaking change.


Of course, the string-based versioning that we used previously is problematic. But if change it, is it not better to use a plain integer, which is easy to compare either in R or C? At the moment there is both R and C code that deals with format versions. The format version isn't really exposed much to users, it's use is primarily technical. Thus I'm not sure that there's any benefit in using multi-part versioning.

What multi-part versioning could achieve is a version bump that keeps the format interpretable by older igraph. While not inconceivable, that is an unusual situation. What information could be added to the format that can be ignored by older versions without loss? The UUID that was added is one example (though its purpose is not 100% clear to me). Another example would be a cache, similar to the one we use in the C core. A cache can be extended in the future, to store more kinds of information, so even a three-part versioning might be useful for it. But do we want to save the cache when saving graphs? Well, is it possible at all to exclude parts of R object when serializing them to a file, or is it mandatory to include everything?

Sorry about thinking aloud while writing.

Yes, perhaps you're right and there's potential value in a multi-part format version after all. We may never use the second part, but keeping the door open for is the smart thing to do. The disadvantage is the difficulty in handling from C. Maybe it's good to fix at least the number of parts?

R/basic.R Outdated Show resolved Hide resolved
@krlmlr krlmlr force-pushed the b-830-stop-downgrade branch 2 times, most recently from a40ea48 to 36c97ae Compare June 10, 2023 13:46
@krlmlr krlmlr changed the title fix: Clean error message when trying to access a graph that has been saved with a newer version fix!: Clean error message when trying to access a graph that has been saved with a newer version Jun 10, 2023
@krlmlr
Copy link
Contributor Author

krlmlr commented Jun 10, 2023

I'm not in love with the three-part version, the version "0.8.0" is meaningless and has only been introduced for igraph 1.0.0 (the version that gained the environment component). The graph_version() function is only used by us (and by the two Leiden packages that are breaking anyway), we could make that return an integer if 0.8.0 is found in the object.

@krlmlr krlmlr changed the title fix!: Clean error message when trying to access a graph that has been saved with a newer version fix!: Clean error message when trying to access a graph that has been saved with a newer or an older version of the package Jun 10, 2023
@krlmlr
Copy link
Contributor Author

krlmlr commented Jun 10, 2023

I backported the tests to the released version of igraph. This PR now shows how the behavior has improved over igraph 1.4.3.

@krlmlr krlmlr changed the title fix!: Clean error message when trying to access a graph that has been saved with a newer or an older version of the package fix!: Clean error message when trying to access a graph that has been saved with a newer or an older version of the package, graph_version() returns an object of class "package_version" Jun 10, 2023
@krlmlr krlmlr merged commit b5234c1 into main Jun 10, 2023
@krlmlr krlmlr deleted the b-830-stop-downgrade branch June 10, 2023 16:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 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.

check_version() and warn_version() claim that the graph has an *older* format even when in fact it's *newer*
2 participants