✨ Combine label files in new combined.json #44
Conversation
scripts/combine.ts
Outdated
| files.forEach((file) => { | ||
| if ( | ||
| file.endsWith(".json") && | ||
| (file === "tokens.json" || file === "accounts.json") |
There was a problem hiding this comment.
Let's make this agnostic for all files in the directory. If we ever add support for blocks or txs, this method will not combine:
There was a problem hiding this comment.
i will replace with endswith(".json") and file!=="combine.json"
scripts/combine.ts
Outdated
| (file === "tokens.json" || file === "accounts.json") | ||
| ) { | ||
| const filePath: string = path.join(dataDir, file); | ||
| const data: Buffer = fs.readFileSync(filePath); |
There was a problem hiding this comment.
If you change this to instead use the FileSystem class we have in the codebase, you don't need to use fs.
There was a problem hiding this comment.
will work on that now
scripts/combine.ts
Outdated
| ) { | ||
| const filePath: string = path.join(dataDir, file); | ||
| const data: Buffer = fs.readFileSync(filePath); | ||
| const jsonData: Array<{ address: string }> = JSON.parse( |
There was a problem hiding this comment.
Can any of these Arrays be replaced with ReadonlyArray?
scripts/combine.ts
Outdated
| } | ||
|
|
||
| function runCombine() { | ||
| fs.readdir(dataFolderPath, (err, files) => { |
There was a problem hiding this comment.
I think this could be done Sync and the fs logic also moved into the FileSystem class
…up typing and merged with new optimism code
kylewandishin
force-pushed
the
feature-combine-data
branch
from
55cfe83
to
4a6dd20
Compare
… a combination of all other combined.json which shows missing records in optimism
scripts/combine.ts
Outdated
| import path from "path"; | ||
| import { FileUtilities } from "./FileSystem/FileSystem"; | ||
|
|
||
| const fs = new FileUtilities(import.meta.url); |
There was a problem hiding this comment.
When you instantiate a class called FileUtilities, it's best practice to call it fileUtilities.
By calling it "fs", it is difficult to know as a future developer if we're using the fs module from node.js
scripts/combine.ts
Outdated
| const providers = fs.readDir(dataFolderPath); | ||
| providers.forEach((provider) => { | ||
| const providerPath: string = path.join(dataFolderPath, provider); | ||
| if (!provider.includes(".")) { |
There was a problem hiding this comment.
When does a provider include a "."? What is a "provider"? I'm not sure if we want to ship this with that variable name. Maybe a comment could help here
scripts/combine.ts
Outdated
| const fs = new FileUtilities(import.meta.url); | ||
|
|
||
| function combineFiles(dataDir: string) { | ||
| const files: ReadonlyArray<string> = fs.readDir(dataDir); |
There was a problem hiding this comment.
Instead of a type-assertion here, what I might prefer here is for fs.readDir to return a ReadonlyArray<string>. Is that possible instead or in-addition?
dawsbot
changed the title
dawsbot
changed the title
Closes #27