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

Bugfix/fixes 251 #465

Merged
merged 9 commits into from
Oct 18, 2023
Merged

Bugfix/fixes 251 #465

merged 9 commits into from
Oct 18, 2023

Conversation

Scotsoo
Copy link
Contributor

@Scotsoo Scotsoo commented Oct 8, 2023

Fixes underlying issue with using path.sep in windows: #251


const PATH_SEPARATOR = path.sep;
const PATH_SEPARATOR = /Windows/i.test(os.release()) ? '\\' : '/';
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use isWindows function from utils/common/platform

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@helloanoop
Copy link
Contributor

@Scotsoo Can you reply to this on the main issue ? #251 (comment)

@helloanoop
Copy link
Contributor

Need some help here.

Can someone test this on windows and confirm?
cc @Its-treason ?

This was the initial fix (fcc12fb) for the phantom folder issue that occurred while renaming

The rename perfectly works in linux and mac. The problem is only on windows.
If this PR fixes the issue, then I would remove the fix we did here fcc12fb in this PR

@Scotsoo
Copy link
Contributor Author

Scotsoo commented Oct 14, 2023

@Scotsoo Can you reply to this on the main issue ? #251 (comment)

Apologies, replied

@Its-treason
Copy link
Member

Its-treason commented Oct 14, 2023

So have been testing around a bit and the Path separator seems to be the underlying issue. So when this fix is merged, fcc12fb can be reverted. The revert can probably be done in this PR.


But before we merge this, I would suggest moving the PATH_SEPERATOR to src/utils/common/platform.js so it is globally in one place. And then remove other PATH_SEPERATOR constants in other places like src/providers/ReduxStore/slices/collections/index.js.

…me/delete needs to be run only on windows"

This reverts commit fcc12fb.

# Conflicts:
#	packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js
#	packages/bruno-app/src/utils/common/platform.js
@Scotsoo
Copy link
Contributor Author

Scotsoo commented Oct 14, 2023

Done!

After reverted changeset + PATH_SEP moved to platform:
electron_nwb4nUSc5B

@Scotsoo
Copy link
Contributor Author

Scotsoo commented Oct 14, 2023

Also deletion works so fixes #265 too
electron_varvRbPR78

})
.catch(reject);
ipcRenderer.invoke('renderer:rename-item', item.pathname, newPathname, newName).then(() => {
dispatch(_renameItem({ newName, itemUid, collectionUid }))
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 the _renameItem call can be removed, because this was basically the original fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@helloanoop helloanoop merged commit 31c4d7e into usebruno:main Oct 18, 2023
@helloanoop
Copy link
Contributor

Merged. Thanks @Scotsoo !

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.

4 participants