Skip to content

Conversation

FelixTheodor
Copy link
Contributor

closes #116

I implemented the features discussed in #116, but I needed to change the "marker" of the ID from # to *. Otherwise, the argument seems to be ignored. I don't know if it's my terminal or the go library or something else, but the arg[0] is empty when the input starts with a #.
This begs two questions: Are you okay with the * as a marker, or would you prefer something else? And should we change the header of the list record command accordingly, to avoid confusion?

@dominikbraun dominikbraun self-requested a review June 18, 2021 17:28
@dominikbraun
Copy link
Owner

I implemented the features discussed in #116, but I needed to change the "marker" of the ID from # to *. Otherwise, the argument seems to be ignored. I don't know if it's my terminal or the go library or something else, but the arg[0] is empty when the input starts with a #.

Ah, yes, because # introduces a shell comment just like // introduces a code comment. I didn't have that in mind. * is not too bad, ! or $ would work out as well... What would @joshuaherrera, @KonstantinGasser and @aligator prefer? 😛

@KonstantinGasser
Copy link
Contributor

I am not sure if $ would work? $ indicates a variable in shell right? At least on iTerm the timetrace edit record $1 returns an accepts 1 arg(s), received 0. Personally I can image that the @ would work good too.

@joshuaherrera
Copy link
Contributor

I implemented the features discussed in #116, but I needed to change the "marker" of the ID from # to *. Otherwise, the argument seems to be ignored. I don't know if it's my terminal or the go library or something else, but the arg[0] is empty when the input starts with a #.

Ah, yes, because # introduces a shell comment just like // introduces a code comment. I didn't have that in mind. * is not too bad, ! or $ would work out as well... What would @joshuaherrera, @KonstantinGasser and @aligator prefer?

I think the * symbol should work fine, I use bash for my terminal and ! is used for history expansion and $ usually refers to a variable, so I think there may be unexpected consequences with those symbols.

@joshuaherrera
Copy link
Contributor

I am not sure if $ would work? $ indicates a variable in shell right? At least on iTerm the timetrace edit record $1 returns an accepts 1 arg(s), received 0. Personally I can image that the @ would work good too.

I like the @ symbol as well, it seems to make the most sense in the context of this command.

@dominikbraun
Copy link
Owner

@KonstantinGasser @joshuaherrera Not sure what was going through my mind when suggesting $ as part of a command syntax 🤣 You're right, @ would be a nice fit. Can we make sure there are no collisions with modules, which are indicated by an @ as well? If yes, I might even prefer @ over *.

@aligator
Copy link
Collaborator

aligator commented Jun 19, 2021

* is treated in some shells as wildcard and some completion tools may replace it with all paths of the current folder... (didn't try that out but who knows what happens....)

So I would also suggest @. In fact the @ is also used for example in systemd to pass params to a service file. systemctl start wg-quick@wg0
So it should work here too.

@dominikbraun dominikbraun added this to the timetrace v0.13.0 milestone Jun 19, 2021
@FelixTheodor
Copy link
Contributor Author

Thanks for all the feedback! I also think that @ is the best solution, therefore I changed it :)

Copy link
Owner

@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

@dominikbraun dominikbraun modified the milestones: timetrace v0.13.0, timetrace v0.14.0 Jul 9, 2021
@dominikbraun
Copy link
Owner

Sorry for the delay - this is going to be merged now!

@dominikbraun dominikbraun merged commit c4ffb78 into dominikbraun:main Sep 12, 2021
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.

Idea: Simplify record keys
5 participants