-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add script to print candle time boundaries #1126
Conversation
WalkthroughThe recent update introduces a new feature to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
indexer/services/scripts/package.json
is excluded by:!**/*.json
Files selected for processing (2)
- indexer/services/scripts/README.md (1 hunks)
- indexer/services/scripts/src/print-candle-time-boundaries.ts (1 hunks)
SERVICE_NAME=script pnpm run print-candle-time-boundaries -- \ | ||
--t 2024-02-28T10:01:36.17+00:00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command addition for print-candle-time-boundaries
is clear and follows the documentation's structure well. However, for consistency with other examples in the documentation, consider adding a brief description above this command to explain its purpose or when it might be used. This can help users understand the context and utility of the script more quickly.
export function calculateNormalizedCandleStartTime( | ||
time: DateTime, | ||
resolution: CandleResolution, | ||
): DateTime { | ||
const epochSeconds: number = Math.floor(time.toUTC().toSeconds()); | ||
const normalizedTimeSeconds: number = epochSeconds - ( | ||
epochSeconds % NUM_SECONDS_IN_CANDLE_RESOLUTIONS[resolution] | ||
); | ||
|
||
return DateTime.fromSeconds(normalizedTimeSeconds).toUTC(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculateNormalizedCandleStartTime
function correctly calculates the normalized start time for a given resolution. It uses Luxon's DateTime
for time manipulation, which is a good choice for handling time zones and formatting. However, consider adding error handling for cases where resolution
might not be in NUM_SECONDS_IN_CANDLE_RESOLUTIONS
, to prevent potential runtime errors.
function getNormalizedBoundaries(time: string, resolutions: CandleResolution[]): void { | ||
const date = DateTime.fromISO(time); | ||
|
||
resolutions.forEach((resolution: CandleResolution) => { | ||
const startTime = calculateNormalizedCandleStartTime(date, resolution); | ||
const endTime = startTime.plus({ seconds: NUM_SECONDS_IN_CANDLE_RESOLUTIONS[resolution] }); | ||
|
||
console.log(`Resolution: ${resolution}, Start Time: ${startTime.toISO()}, End Time: ${endTime.toISO()}`); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getNormalizedBoundaries
function effectively iterates over resolutions, calculates start and end times, and logs them. This function is clear and concise. However, it's important to ensure that the input time
string is in a valid ISO format before converting it with DateTime.fromISO
. Consider adding a validation step or try-catch block to handle parsing errors gracefully.
const args = yargs.options({ | ||
time: { | ||
type: 'string', | ||
alias: 't', | ||
description: 'Time to compute normalized boundaries for, e.g. 2024-02-28T10:01:36.17+00:00', | ||
required: true, | ||
}, | ||
}).argv; | ||
|
||
getNormalizedBoundaries(args.time, resolutions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script uses yargs
for command-line argument parsing, which is a robust choice for Node.js CLI applications. The time
argument is correctly marked as required and has a clear description. To enhance user experience, consider adding an example usage of the script in the help text or documentation, demonstrating how to format the time
argument correctly.
Changelist
Add script to print candle time boundaries
Test Plan
Ran locally
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.