Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

New command : create new post #284

Merged
merged 7 commits into from
Dec 13, 2015
Merged

New command : create new post #284

merged 7 commits into from
Dec 13, 2015

Conversation

k94ll13nn3
Copy link

This is an first draft of the command. This is not yet finished and I have some questions regarding the implementation.

First, in the current state of the commands in Pretzel, if I use the pretzel.exe newpost "A new post in Pretzel" syntax for the command, it prevent me from using the parameters.Path variable because it will contains the title (because of the SetPath method). This means that the -s option will not be usable too. I could create a new option instead, but in my opinion, in this case, the default argument will be the title and not the path. So I'm not sure what is the best way to do it.

Second, I added all the command logic the the SpiceCommand.cs file but it is not ideal for testing. So I was wondering if I should put the work in a file in the Pretzel.Logic project (in which folder?) and call this class from the command.

Finally, I tried to find a name that continue the cooking theme but I'm not sure that I came up with the best name :)

@laedit
Copy link
Member

laedit commented Dec 7, 2015

Excellent, thanks 😃

About the arguments, I think that we should be coherent with the existing commands: add a new parameter -t - --title. And by default the title could be "New post" or something like that.
A little warning, you have added the draft option to the command writer but there is only the drafts option existing.

You are right about the location of the class, it should be in the Logic project. Could you move it to the Logic project and all necessary interface, in order to not have to just reference a class in Logic but have the command directly?

About the name, I think that spice is more related to the plugin, the posts are the base of pretzel, without it no cooking is possible 😄
What do you think about Ingredient?
To go further, I think that a new command should be good, allowing to add posts, draft or page for now, but maybe helping to create new plugin (spice 😉) in the future. What do you think of that?

@k94ll13nn3
Copy link
Author

I have added the -n - --newposttitle parameter. I wanted to use -t - --title but it was incorrectly catch by the method that display the help (because of the template parameter).

I have changed the name to Ingredient, moved the class in the Pretzel.Logic project and added some tests for the command.

@laedit
Copy link
Member

laedit commented Dec 11, 2015

Thanks for the modification, but I still have some remarks/questions:

  • you didn't tell me what you think about a new command, don't you think it will be more appropriate? (pretzel new ingredient "my awesome post")
  • instead of -n / --newposttitle I think --posttitle would be less confusing. Sadly the -p or -t are already taken. The commands organization is really bad right now, we need to improve that too.
  • have you tried to move ICommand to Pretzel.Logic? I think that it could be interesting to provide that as extensiblity. But it can be done another time.

@k94ll13nn3
Copy link
Author

  • I did not understand your comment like that. I think that a pretzel new ingredient "my awesome post" command might be more appropriate, and it will be easier to had new tasks related to add something instead of recreating a command each time (like the new plugin that you mentioned). But it may require some modifications in the parameter processing to avoid a pretzel new --task ingredient --title "my awesome post" like command, which I find a bit heavy.
  • Yeah, I wanted to use --posttitle too, but like you said, -p was already taken.
  • Again did not understand that you wanted to move the ICommand too, I think it shouldn't be hard to do it, but maybe it will be better in a new PR, to keep this one focus on the command. What do you think?

@laedit
Copy link
Member

laedit commented Dec 11, 2015

For the ICommand part, it's just the will to have all the logic part in Pretzel.Logic, and allow the plugins creator to add new command. But you are right, I'll do that another time.

About the new command, I see that as a sub command with a bunch of pre-defined commands. But I haven't verified if it's doable in Pretzel right now and I'm afraid it's not the case.
I wanted to do that in order to not be stuck with weird command like for this PR the n parameter which represent the title of the post...
But since doing that has a risk to impact all commands system, I'm ok to go with your current Pretzel ingredient command for now, and maybe modify it as Pretzel new ingredient later when the commands system will be modifiable.


parameters.Parse(arguments);

var title = parameters.NewPostTitle;
Copy link
Member

Choose a reason for hiding this comment

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

title is not used, could you delete it?

@laedit
Copy link
Member

laedit commented Dec 13, 2015

I was about to merge the PR but there is still a modification to do.

@k94ll13nn3
Copy link
Author

What did I forget to change/remove ?

@laedit
Copy link
Member

laedit commented Dec 13, 2015

I added a comment about the title variable in IngredientCommand.

@k94ll13nn3
Copy link
Author

I changed it in 6718e58, should have added a comment, sorry 😳

laedit pushed a commit that referenced this pull request Dec 13, 2015
@laedit laedit merged commit 6ae0e48 into Code52:master Dec 13, 2015
@k94ll13nn3 k94ll13nn3 deleted the newpost-command branch December 13, 2015 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants