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

clone functionality in collection #1202

Merged
merged 5 commits into from
Dec 17, 2023

Conversation

akshat-khosya
Copy link
Contributor

@akshat-khosya akshat-khosya commented Dec 11, 2023

Description

Issue: [Feature]-collection clone or duplicate feature/option is not available #156
I have created clone collection functionality. I just created new folder at user location and copied previous collection files.
Here is demo video of that.
Screencast from 11-12-23 02:11:38 AM IST.webm

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

Publishing to New Package Managers

Please see here for more information.

Copy link
Member

@Its-treason Its-treason left a comment

Choose a reason for hiding this comment

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

Awesome job on your first contribution! There are a couple of areas that need tweaks, but looks very good overall. Keep up the good work!

// clone collection
ipcMain.handle(
'renderer:clone-collection',
async (event, collectionName, collectionFolderName, collectionLocation, perviousPath) => {
Copy link
Member

Choose a reason for hiding this comment

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

Typo in perviousPath


await createDirectory(dirPath);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if you split the code into 3 or 4 logical parts / blocks by adding an empty line between them.

And remove the console.log's

await writeFile(path.join(dirPath, 'bruno.json'), cont);
const files = searchForBruFiles(perviousPath);
console.log(files);
for (const sourceFilePath of files) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to handle directories correctly. You could try to use "fs.cpSync(perviousPath, dirPath, { recursive: true });" here.

grafik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I missed that case to handle file dir to clone collection location

mainWindow.webContents.send('main:collection-opened', dirPath, uid, json);
ipcMain.emit('main:collection-opened', mainWindow, dirPath, uid);
} catch (error) {
return Promise.reject(error);
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the try/catch because you are not doing anything with the error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I think I have copied the create collection code, but that is correct it do nothing.

Comment on lines 945 to 950
return new Promise((resolve, reject) => {
ipcRenderer
.invoke('renderer:clone-collection', collectionName, collectionFolderName, collectionLocation, perviousPath)
.then(resolve)
.catch(reject);
});
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify this code here, ipcRenderer.invoke() already returns a promise.

Suggested change
return new Promise((resolve, reject) => {
ipcRenderer
.invoke('renderer:clone-collection', collectionName, collectionFolderName, collectionLocation, perviousPath)
.then(resolve)
.catch(reject);
});
return ipcRenderer.invoke('renderer:clone-collection', collectionName, collectionFolderName, collectionLocation, perviousPath)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have corrected this

Copy link
Member

Choose a reason for hiding this comment

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

Hey, i think you need to look at this again. Now the modal always stays open, because the promise is never resolved.

I think this would make more sense:

export const cloneCollection = (collectionName, collectionFolderName, collectionLocation, perviousPath) => () => {
  const { ipcRenderer } = window;

  return ipcRenderer.invoke(
    'renderer:clone-collection',
    collectionName,
    collectionFolderName,
    collectionLocation,
    perviousPath
  );
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, now it is working fine

@akshat-khosya
Copy link
Contributor Author

@helloanoop Can you please merge my PR.

@helloanoop helloanoop merged commit cb6513c into usebruno:main Dec 17, 2023
2 checks passed
@helloanoop
Copy link
Contributor

Merged!

Nice work, @akshat-khosya !
Thanks for the review @Its-treason !

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

3 participants