Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Remove normalizeFilename function #13147

Merged
merged 2 commits into from
Mar 6, 2017
Merged

Remove normalizeFilename function #13147

merged 2 commits into from
Mar 6, 2017

Conversation

ficristo
Copy link
Collaborator

@ficristo ficristo commented Mar 3, 2017

Quick hack for #12744, I doubt this is the correct way to fix.
I am only interested to see if this could solve the crashes people are experiencing.

/cc @zaggino

@zaggino
Copy link
Contributor

zaggino commented Mar 3, 2017

This construct

var fileName = fileInfo.name;
if (ternServer.projectDir && fileName.indexOf(ternServer.projectDir) === -1) {
  fileName = ternServer.projectDir + fileName;
}

repeats itself if I have counted right and not missed any difference 5 times over the diff, perhaps

function getNormalizedFilename(fileInfo, ternServer) {
  return ...
}

and use var fileName = getNormalizedFilename(fileInfo, ternServer); would be a bit nicer

@ficristo
Copy link
Collaborator Author

ficristo commented Mar 3, 2017

Sure! If this fixes the crashes I'll refactor like that.

@zaggino
Copy link
Contributor

zaggino commented Mar 3, 2017

Anyway, were you able to replicate anything, or is this just a hinch? If it doesn't break any tests I think we could merge this for time being as #11948 doesn't seem to be moving much...

@ficristo
Copy link
Collaborator Author

ficristo commented Mar 3, 2017

Since people reported crashes in 1.8 that weren't in 1.7, this is the only thing that comes to my mind.
So yes, this is only an hinch...
The tests pass.

@zaggino
Copy link
Contributor

zaggino commented Mar 3, 2017

I have a project which, while working on it, definitely crashes Brackets, but I can't replicate this. Sometimes after 5 minutes, sometimes after an hour. So it'd vote the approach of merging it and seeing if there's any change in the reports.

Copy link
Contributor

@zaggino zaggino left a comment

Choose a reason for hiding this comment

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

refactor before merge

@ficristo
Copy link
Collaborator Author

ficristo commented Mar 4, 2017

Refactored. After running tests again, they are quite flaky. But I'm not sure if it is because our testsuite or because my PR...

@@ -587,14 +588,24 @@ var config = {};
* @param {string} path - the path of the file
*/
function handlePrimePump(path) {
var fileInfo = createEmptyUpdate(path),
var fileName = path;
if (ternServer.projectDir && fileName.indexOf(ternServer.projectDir) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if you don't mind one last change, for future readability, I'd extract this to _getDenormalizedFilename as this basically reverts _getNormalizedFilename if I'm not mistaken

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@zaggino
Copy link
Contributor

zaggino commented Mar 6, 2017

Hi @ficristo , I've run the tests and they all pass on my machine so that thing is fine.
Just one minor thing (commented above) and I'd merge this if nobody doesn't want to add anything.

@zaggino zaggino merged commit 9a60020 into adobe:master Mar 6, 2017
@zaggino
Copy link
Contributor

zaggino commented Mar 6, 2017

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants