Navigation Menu

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

Add MODIFY to /network, /server and /channel #338

Closed
wants to merge 11 commits into from
Closed

Add MODIFY to /network, /server and /channel #338

wants to merge 11 commits into from

Conversation

vague666
Copy link
Member

Since people are confused about ADD working as modify, lets make it more clear

@dequis
Copy link
Member

dequis commented Oct 10, 2015

Dunno if i like this idea, but at least document that ADD does the same thing if the network already exists, instead of removing that line completely.

Also MODIFY should fail if the network/server/channel doesn't exist instead of being exactly the same thing as add

@vague666
Copy link
Member Author

Yeah, I was thinking about that, but since it is just an alias to make it more clear to users I thought the duplication of code would be more work than warranted
But sure, I can add cmd_server_modify & co if you want that.
Or I can amend the help files with info that modify is just an alias for add

@ahf
Copy link
Member

ahf commented Oct 21, 2015

I'm not fully convinced about this. I think this is a documentation issue.

@vague666
Copy link
Member Author

It is a documentation issue also, but adding modify doesn't add much code and it would make it more clear to users what to do. This PR is mostly help modifications but it's easy to overlook documentation when you are looking for a particular thing.

@ailin-nemui ailin-nemui modified the milestone: 0.8.19 Nov 1, 2015
@ailin-nemui
Copy link
Contributor

I'm not very much in favour of this unless you actually modify MODIFY to disable its ADD functionality. (I'd keep ADD as fully functional modify for backward compat)

@ailin-nemui
Copy link
Contributor

I like the direction your code is taking, however you will need to refactor the common code parts into static methods instead of using copypasta

@ailin-nemui
Copy link
Contributor

ailin-nemui commented Jun 13, 2016

hi @vague666 , I really like the change you're proposing even if it is conceptually minor. however there is still work left before I can accept it. You know what I've been thinking about may be better instead:

  • take the existing _add functions, rename them to from cmd_..._add to ..._add_modify with a final argument gboolean add
  • only slightly modify them each, changing if (rec == NULL) { to if (rec == null && !add) { error out } else if (rec == NULL && add) { original code goes on
  • mask each line that should not be applicable to modify in such a way
  • create cmd_..._add and cmd_..._modify that call ..._add_modify(..., true/false)

That would also immediately get rid of some of the code duplication I marked with A and B

g_free_and_null(rec->own_host);
rec->own_ip4 = rec->own_ip6 = NULL;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

B

@vague666 vague666 closed this Jun 18, 2016
@ailin-nemui ailin-nemui modified the milestones: 0.8.21, 1.0.1 Jan 3, 2017
@ailin-nemui ailin-nemui added this to the 1.0.0 milestone Jun 26, 2019
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.

None yet

4 participants