Skip to content

Support tracking apps#98

Merged
mschenk42 merged 9 commits intogit-time-metric:masterfrom
desaroger:feature/support-tracking-apps
Jun 30, 2019
Merged

Support tracking apps#98
mschenk42 merged 9 commits intogit-time-metric:masterfrom
desaroger:feature/support-tracking-apps

Conversation

@desaroger
Copy link
Copy Markdown
Contributor

@desaroger desaroger commented Jun 24, 2019

I wrote an issue (#96) some days ago, and this PR is my attempt to implement it myself.
This is my first time programming in Go, so any help/advice will be appreciated, no hard feelings, be honest haha

Summary

So, basically I added the option --app to the record command, which indicates that the string passed is not a file, but an app name. Then it creates (if does not exists yet) the file .gtm/{appName}.app and tracks a simple event to that file. It's basically the same as the way the terminal was implemented, but for any app.

Also, added the option --app-off to some other commands, mimicking the --terminal-off behaviour.

What about terminal support

My idea was to try to "remove" the code related with the terminal support, but make the new code to work for the terminal in the exact same way. I did this for the templates, for the record command, etc. However I keep all the command flags, so everything keeps working as before for the terminal.

I wanted to add a "deprecated" warning when using the terminal flags, but I think is not my decision. Though I think the new app support makes the terminal flags to not make a lot of sense (my opinion).

You can mimic the terminal record command gtm record -terminal with the more generic flag gtm record -app terminal. It does exactly the same thing. I even changed the implementation of the former as an alias of the latter.

Tests

I added some test cases, basically where there were tests for terminal support. Everyhing builds and pass the tests for me, but the lint tests (make lint) throws lots of warnings. But I think is a problem of my Go setup, because those warning were thrown even with a clean cloned repo.

Things changed

I tried my best to not change anything of the current behavior, and unless I did something wrong, the only things that have been change:

  • The command's help, where I add the new options related with apps
  • The terminal row of the report and status outputs. I wanted a way to differentiate the apps from the files. Currently, the terminal showed up as "Terminal". But in my testings, with multiple apps, it was a little mess to have everything mixed, so I ended up adding a "[app]" at the beginning of any app, including the terminal. I am not super happy with that, I think the correct way to do it would be to have an extra column to indicate the type of the file/event, so it would be 'file' for files, but 'app' for apps, and we can add extra types in the future. But I was afraid of changing the current behavior of the templates so ended up only adding '[app]', and I will wait so you guys say your opinion.

I hope I didn't made some big mistake. I will change anything if you see a better way of doing it.

Thanks!

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 24, 2019

Coverage Status

Coverage increased (+1.2%) to 65.178% when pulling 6e8c12b on desaroger:feature/support-tracking-apps into 8e616f8 on git-time-metric:master.

@mschenk42
Copy link
Copy Markdown
Member

Thanks for the PR @desaroger. This looks pretty good. I'll give this a more thorough review by the end of the week.

@mschenk42 mschenk42 self-requested a review June 24, 2019 23:55
@mschenk42 mschenk42 merged commit 5cd6bd1 into git-time-metric:master Jun 30, 2019
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.

3 participants