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

utils: add AzFsExtraFile.appendFile #1525

Merged
merged 12 commits into from
Jul 18, 2023
Merged

utils: add AzFsExtraFile.appendFile #1525

merged 12 commits into from
Jul 18, 2023

Conversation

nturinski
Copy link
Member

No description provided.

@nturinski nturinski requested a review from a team as a code owner July 18, 2023 22:02
@@ -73,6 +73,12 @@ export namespace AzExtFsExtra {
await workspace.fs.writeFile(uri, Buffer.from(contents));
}

export async function appendFile(resource: Uri | string, contents: string): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

maybe a better name is appendToFile?

Copy link
Member Author

@nturinski nturinski Jul 18, 2023

Choose a reason for hiding this comment

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

Mimicking this https://www.geeksforgeeks.org/node-js-fs-appendfile-function/ so we can have a 1:1 conversion.

Copy link
Member

@alexweininger alexweininger left a comment

Choose a reason for hiding this comment

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

Do you need to add the method to the declaration file?

@nturinski nturinski merged commit 6ddea7d into main Jul 18, 2023
4 checks passed
@nturinski nturinski deleted the nat/ui/appendFile branch July 18, 2023 23:41
@nturinski
Copy link
Member Author

Ah crap, I was thinking of the exports file. Yeah, I need to do that (and will do that with the bump)

export async function appendFile(resource: Uri | string, contents: string): Promise<void> {
const uri = convertToUri(resource);
const existingContent = await AzExtFsExtra.readFile(uri);
await AzExtFsExtra.writeFile(uri, existingContent + '\r\n\r\n' + contents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add the newlines though? Seems like separator should be the decision of the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have to, I was emulating what fs.appendFile does though. We could have an option for "separator" though, I'd like to default to \r\n\r\n since that is the default behavior of fs.appendFile.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good middle ground.

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.

3 participants