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

285: Pass interval id to extensions #286

Merged
merged 1 commit into from Mar 3, 2020

Conversation

j6s
Copy link
Contributor

@j6s j6s commented Feb 11, 2020

Passes the id of an interval to extensions. Now, stdin of an extension looks like this:

{"id":99,"start":"20191219T183718Z","end":"20191219T185724Z","annotation":"project:gaming"},
{"id":98,"start":"20191219T211035Z","end":"20191219T220034Z","tags":["project:gaming"]},
{"id":97,"start":"20200107T090000Z","end":"20200107T113235Z","tags":["tempo:ABC-477"]},
{"id":96,"start":"20200107T125607Z","end":"20200107T152439Z","tags":["tempo:ABC-477"]},
{"id":95,"start":"20200107T165000Z","end":"20200107T180803Z","tags":["tempo:ABC-477"]},
{"id":94,"start":"20200108T081022Z","end":"20200108T083142Z","tags":["tempo:ABC-477"]},

Resolves #285

Copy link
Contributor

@sruffell sruffell left a comment

Choose a reason for hiding this comment

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

The only other thought I have is that I wonder if this should only be enabled when the :ids hint is specified on the command line? This would be consistent with timew summary but most consumers of the JSON should probably only be looking for specific keys and would ignore the id tag by default.

src/Interval.cpp Outdated Show resolved Hide resolved
src/Interval.cpp Outdated
Comment on lines 107 to 109
if (is_ended ())
{
if (is_started ())
out << ',';
out << "\"end\":\"" << end.toISO () << "\"";
}
out << ",\"end\":\"" << end.toISO () << "\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too you'll want to add curly braces.

Copy link
Member

@lauft lauft Feb 12, 2020

Choose a reason for hiding this comment

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

I know that there rarely is an interval without a start, but I would prefer to keep lines 109 - 110 for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand lines 109-110 were adding a comma in front only if the start existed because if a start existed, the start would be the first item in the json object but if a start did not exist, the end time would be the first item in the object.

With this change, there is always a defined first item in the object: the id.

Copy link
Member

Choose a reason for hiding this comment

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

@j6s you are definitely right here 😅 - however I found that there is the case of an empty interval which then gets serialized as {"id":0} instead of {}.
And also tests dom.t and interval.t are broken. They appear green on Travis (should be fixed with dcfe0d1c), but if you inspect the individual runs you see the error reports (e.g. https://travis-ci.org/GothenburgBitFactory/timewarrior/jobs/651153570)

@lauft lauft added the enhancement New feature or request label Feb 29, 2020
Copy link
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

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

Empty interval case needs to be addressed
Please check/adapt tests dom.t and interval.t

@lauft
Copy link
Member

lauft commented Mar 3, 2020

I discovered that this feature comes handy for tests, also in context of #294. I will merge it and handle the open issues mentioned in the review. Thanks @j6s nevertheless for this contribution!

@lauft lauft merged commit da27fdc into GothenburgBitFactory:dev Mar 3, 2020
@j6s
Copy link
Contributor Author

j6s commented Mar 3, 2020

Sorry for not finishing this - I had genuinely planned to have a look at it on the weekend.

@lauft
Copy link
Member

lauft commented Mar 3, 2020

@j6s Don't be. You can take a look at the documentation and see whether (or better where) this needs to be updated. And, also important, consider to publish your addon on github/... and tell us about it so we can mention it to other users 😉

@lauft lauft added this to the 1.2.1 milestone Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass record id to extensions
3 participants