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

Added rename #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added rename #2

wants to merge 3 commits into from

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