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

Added a check of Untitled doc in _getNormalizedFilename and _getDenormalizedFilename #13201

Merged
merged 1 commit into from
Mar 21, 2017

Conversation

saurabh95
Copy link
Contributor

@saurabh95 saurabh95 commented Mar 21, 2017

Actually in the normalize path earlier there was no check of Untitled doc and it was appending the project dir to the Untitled path, so I added a check for Untitled doc before appending the project root.
Due to this JS codehints were not working for Untitled documents on changing the mode to JS.

All JavaScriptCodeHints tests are passing

@saurabh95 saurabh95 added this to the Release 1.9 milestone Mar 21, 2017
@ficristo
Copy link
Collaborator

Since this code will be refactored probably it is better to add a test.
Also you need to retarget the branch to release (if there aren't plans to merge again master to release of course).
Looking at the surrounding code of tern-worker.js usually parameters are passed to the functions instead of using a global variable like isUntitledDoc (see handleUpdateFile for example) but I don't think it is a big deal.
I added these two functions in #13147 in order to be able to remove normalizeFilename override. It's not pretty and if there are ways to improve the situation I would only be happy.

Overall this seems ok to me.

@saurabh95
Copy link
Contributor Author

saurabh95 commented Mar 21, 2017

@ficristo I will definitely add test after the release activities. Also will try to refactor the code as well.
Could you merge this PR if the changes seem ok to you?

@ficristo
Copy link
Collaborator

You can go ahead and merge for me.
Since I think you should retarget this PR to release branch I leave the merge to you.

@swmitra
Copy link
Collaborator

swmitra commented Mar 21, 2017

@saurabh95 I am fine with the changes to unblock us for the release now, but would like to see some tests being added in general for InMemory document use cases (not just JS code hints). Also note that, eventually we have to refactor and migrate this code to Tern on node as well so that we can merge that PR on master after brackets 1.9 release. We may need more changes to leverage on new features/APIs added in Tern.

@swmitra
Copy link
Collaborator

swmitra commented Mar 21, 2017

Merging.

@swmitra swmitra merged commit aefa3ab into master Mar 21, 2017
Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

LGTM

@swmitra
Copy link
Collaborator

swmitra commented Mar 21, 2017

@petetnt We both were typing together I guess 😄

@swmitra
Copy link
Collaborator

swmitra commented Mar 21, 2017

@ficristo Let him do cherrypick after merging this to master, to keep master and release even for this module 👍

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.

4 participants