Skip to content

Conversation

@FluffyStuff
Copy link

Same as gobby/gobby#7

@aburgm
Copy link
Contributor

aburgm commented Jul 12, 2014

Thanks for the contribution! This looks mostly good and this is a very welcome contribution. I quickly looked over the patches and noticed only minor issues. I'll hopefully go through them more throughly later today or tomorrow. Would you mind rebasing the commits such that there is only one commit on top of libinfinity master? This would make it a bit easier for me to review them.

@aburgm
Copy link
Contributor

aburgm commented Jul 12, 2014

I have commented on individual lines of the patch. In general it looks very good, and these are mostly minor issues. Thanks again for the contribution! Can you fix these and update the pull request? In addition, the newly added API functions would need to be added to `docs/reference/libinfinity/libinfinity-0.6-sections.txt, so that they show up in the API documentation:

inf_browser_rename_node
inf_browser_node_renamed
infd_storage_rename_node
inf_request_result_make_rename_node
inf_request_result_get_rename_node
inf_file_util_rename

Can you clarify what additional signal you intent to create, and for what reason? In principle the InfSession object should not need to care about renaming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants