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

[CLOSED] Added a check of Untitled doc in _getNormalizedFilename and _getDenormalizedFilename #11199

Open
core-ai-bot opened this issue Aug 30, 2021 · 7 comments

Comments

@core-ai-bot
Copy link
Member

Issue by saurabh95
Tuesday Mar 21, 2017 at 06:13 GMT
Originally opened as adobe/brackets#13201


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 included the following code: https://github.com/adobe/brackets/pull/13201/commits

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Mar 21, 2017 at 08:56 GMT


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 adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by saurabh95
Tuesday Mar 21, 2017 at 08:59 GMT


@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?

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Mar 21, 2017 at 09:12 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Mar 21, 2017 at 09:14 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Mar 21, 2017 at 09:15 GMT


Merging.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Mar 21, 2017 at 09:15 GMT


@petetnt We both were typing together I guess 😄

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Mar 21, 2017 at 09:17 GMT


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

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

No branches or pull requests

1 participant