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

Fix for issue #30 #35

Merged
merged 3 commits into from
Jul 7, 2015
Merged

Fix for issue #30 #35

merged 3 commits into from
Jul 7, 2015

Conversation

David-Polehonski
Copy link
Contributor

Added editor listeners to set .cfm files to html/cfml mixed mode if the file contains non cfml tags.

@dajester2013
Copy link
Contributor

I would think it would be better to have the html grammar added back to cfml.cson, as cfml tags are intended to be interspersed with html.

The only thing I don't like about having to scan for mixed mode is if you run into a situation where you are scanning hundreds of lines on every change to the file (the event is slightly buffered, but every keystroke runs the did-change event).

@David-Polehonski
Copy link
Contributor Author

@dajester2013 We could only scan the file on open? I wouldn't remove the cfml only mode, but we could remove the fileTypes array? the cfml only mode allows for a separation of concerns and I find it particularly useful when testing.

@dajester2013
Copy link
Contributor

Scanning the file on change allows someone to open a blank cfc file with the CFML grammar active, then start typing component and have it switch automatically to CFScript. You could assume that the default for cfc's is cfscript, except when someone starts typing <cfcomponent>, it would not automatically switch over to cfml if the scanning was only on file open.

I was reserved on adding the file change listener to begin with, that's why I only looked for the first non-empty line.

@dajester2013 dajester2013 mentioned this pull request Jul 7, 2015
@David-Polehonski
Copy link
Contributor Author

I just peaked at the docs for the did-change, it passes a delta containing the line/s that have been changed. Think we could write it to only update based on the actual changed lines? I.e if they type <cfcomponent on line four the delta reads {'start':4, 'end':4} so we could just read that line to trigger a switch?

@dajester2013
Copy link
Contributor

I don't know if that would work. The grammar should only be determined based on the first line.

I originally used a regex scan, but I had a problem where if I typed something like:

import a.b.c;

<cfcomponent>

it would start switching back and forth between the cfml and cfscript grammars rapidly. I think that would be the case if it were only inspecting the deltas.

@David-Polehonski
Copy link
Contributor Author

Well removing the fileTypes array seems to have done the trick regarding the HTML mixed mode - at least it seems to be working for me. With that being the case then I think your initial first line scan should be sufficient to most use cases (Read: I can't think of one where it fails).

@atuttle
Copy link
Owner

atuttle commented Jul 7, 2015

Sorry, I'm quite busy today and having some trouble keeping up with all of the conversations. Is this PR ok to merge, then?

@dajester2013
Copy link
Contributor

I'm good with it.
On Jul 7, 2015 1:05 PM, "Adam Tuttle" notifications@github.com wrote:

Sorry, I'm quite busy today and having some trouble keeping up with all of
the conversations. Is this PR ok to merge, then?


Reply to this email directly or view it on GitHub
#35 (comment)
.

atuttle pushed a commit that referenced this pull request Jul 7, 2015
@atuttle atuttle merged commit 7c0f71b into atuttle:master Jul 7, 2015
@atuttle
Copy link
Owner

atuttle commented Jul 7, 2015

$ apm publish patch
Preparing and tagging a new version ✓
Pushing v0.8.3 tag ✓
Publishing language-cfml@v0.8.3 ✓

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