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

fix: minor history fixes #566

Merged
merged 1 commit into from Nov 30, 2023
Merged

Conversation

rossiam
Copy link
Collaborator

@rossiam rossiam commented Nov 29, 2023

  • treat integer values for before and after flags as seconds since the epoch as describe in the help
  • refactor: simplified sort function
  • refactor: minor reformatting
  • refactor: use confirm inquirer input type for y/n question instead of text input
  • in JSON/YAML output, strictly obey after
    • the API first returns items after the specified after but allows the user to continue on with the "next" page link
    • the interactive table format output still allows the user to continue on to view items, as the API does
    • when asking for JSON or YAML output, we now strictly obey the after flag

Checklist

  • I have read the CONTRIBUTING document
  • Any required documentation has been added
  • My code follows the code style of this project (npm run lint produces no warnings/errors)
  • I have added tests to cover my changes

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: de1a160

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@smartthings/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

break
}

const table = command.tableGenerator.newOutputTable({ isList: true, head })
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know you could re-define a const in a more restricted context like this. Neat. But could certainly get confusing in a less straightforward situation than this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is something you should generally avoid because it can be confusing. I decided to go with it in this case since both variables are created and used in just a few lines of code (and I really wanted that same name for both of them since they do the same thing) but I wouldn't fight if you wanted me to change it.

@@ -132,6 +131,11 @@ export const getHistory = async (client: SmartThingsClient, limit: number, perRe
const items: DeviceActivity[] = [...history.items]
while (items.length < limit && history.hasNext()) {
await history.next()
// The API allows the user to continue to records before the specified after with paging so
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording is a little hard to follow. I think it's "continue to records before the specified after" that confuses me--maybe something like "continue to retrieve records before the specified 'after' date"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confusing? That's an understatement. Mangled is more like it. 😄 I must have messed this in editing. I've updated it to:

		// The API allows the user to continue to view history from before the specified "after"
		// with paging so we stop processing if we get items that come before the specified "after".

@rossiam rossiam merged commit fdf4250 into SmartThingsCommunity:main Nov 30, 2023
4 checks passed
@rossiam rossiam deleted the history-fixes branch November 30, 2023 18:36
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.

None yet

2 participants