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

Improvement of commandline processing #1025

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

Conversation

FDUTCH
Copy link
Contributor

@FDUTCH FDUTCH commented Mar 5, 2025

with this change you can put as much spaces as you want into the command
something like this: /tell "Cool guy" hello! will not result an error any more

@Sandertv
Copy link
Member

Sandertv commented Mar 5, 2025

This is not valid in vanilla and I don't really see the need for this to be valid in dragonfly. Any motivation for this change?

@FDUTCH
Copy link
Contributor Author

FDUTCH commented Mar 5, 2025

This is not valid in vanilla and I don't really see the need for this to be valid in dragonfly. Any motivation for this change?

"This is not valid in vanilla" are you sure?

@Sandertv
Copy link
Member

Sandertv commented Mar 5, 2025

Looks like they've changed this. It is actually valid now. Point taken.

@@ -345,3 +335,41 @@ func exportedFields(command reflect.Value) []reflect.StructField {
}
return fields
}

// SplitCommandArgs splits str to args for command execution.
func SplitCommandArgs(str string) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Function should not be exported.

Copy link
Member

Choose a reason for hiding this comment

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

I realise that this is used in player.go, but we should probably adapt that in one way or another so that this does not require being exported.

Comment on lines 348 to 370
for _, char := range str {
switch char {
case '"':
quote = !quote
case ' ':
if quote {
// writing part of the string.
arg.WriteRune(char)
continue
}
if previousSpace {
// trimming space.
continue
}
// end of the arg.
previousSpace = true
args = append(args, arg.String())
arg.Reset()
default:
previousSpace = false
arg.WriteRune(char)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of re-inventing an entire parser for this. My suggestion would be this:

  • Keep using the csv reader, merge the first value in the slice returned with the second value if first value == "/"
  • Remove all empty strings from the slice returned, except if the value is last in the slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a huge fan of re-inventing an entire parser for this. My suggestion would be this:

* Keep using the csv reader, merge the first value in the slice returned with the second value if `first value == "/"`

* Remove all empty strings from the slice returned, except if the value is last in the slice.

yeah maybe writing own parser is not a very good idea

@FDUTCH
Copy link
Contributor Author

FDUTCH commented Mar 5, 2025

still it's not fully vanilla behavior cause vanilla allows to write commands like so / tell someone something, but it is better than current system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants