Skip to content

Feature add new bin format#9

Open
xavi-pinsach wants to merge 6 commits intomasterfrom
feature_add_new_bin_format
Open

Feature add new bin format#9
xavi-pinsach wants to merge 6 commits intomasterfrom
feature_add_new_bin_format

Conversation

@xavi-pinsach
Copy link
Copy Markdown
Contributor

No description provided.

@xavi-pinsach xavi-pinsach requested a review from jbaylina June 14, 2022 05:16
Comment thread package.json
Comment thread .github/workflows/ci.yml
with:
node-version: ${{ matrix.node-version }}
check-latest: true
#cache: 'npm'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#cache: 'npm'
cache: 'npm'

The actions team said that the cache problem has been resolved

Comment thread src/binfileutils.js
await readBinFileV2();
}

fd.binVersion = binVersion;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you add the binVersion to the fd instead of return it in the object below?

I could see the return value be { fd, sections, binVersion } instead. Thoughts?

Comment thread src/binfileutils.js

return {fd, sections};

async function readBinFileV1() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you want these functions inside the closure instead of passing in arguments you need?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should only be used inside readBinFile(...).

Comment thread src/binfileutils.js
}
}

async function readBinFileV2() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you want these functions inside the closure instead of passing in arguments you need?

Comment thread src/binfileutils.js
}
}

fd.binVersion = binVersion;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same question as above. We could return { fd, binVersion } below

Comment thread src/binfileutils.js
throw new Error("Already writing a section");
}

if(BIN_FORMAT_1 === fd.binVersion) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see that you didn't want to change the parameters here, which only works if you attached binVersion to the fd 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a flag for the file descriptor indicating which binary format version is using

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.

2 participants