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 a case when extension less file is importing other less files #7230

Merged
merged 4 commits into from Mar 19, 2014

Conversation

Projects
None yet
3 participants
@zaggino
Contributor

zaggino commented Mar 18, 2014

I wanted to split less file my extension is using into more files, but loading failed when using valid import directives. Through debugging, I found out that some properties of file info were not filled as it should.

These are the values for file brackets.less:

currentDirectory: "file:///D:/Program%20Files/Brackets/dev/src/styles/"
entryPath:        "file:///D:/Program%20Files/Brackets/dev/src/styles/"
filename:         "file:///D:/Program%20Files/Brackets/dev/src/styles/brackets.less"
relativeUrls:     undefined
rootFilename:     "file:///D:/Program%20Files/Brackets/dev/src/styles/brackets.less"
rootpath:         "file:///D:/Program%20Files/Brackets/dev/src/styles/"

And these are the original values calculated for brackets-git.less:

currentDirectory: ""
entryPath:        ""
filename:         "brackets-git.less"
relativeUrls:     undefined
rootFilename:     "brackets-git.less"
rootpath:         "file:///C:/Users/Zaggi/AppData/Roaming/Brackets/extensions/user/zaggino.brackets-git/less/"

The fix makes fileInfo look the same way as in the brackets.less case and all references are loaded correctly.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

Btw, is there a reason Brackets is still using less 1.4.2 when latest version is 1.7.0 ? The fix works for both versions and I didn't find any ill effects when upgrading to 1.7.0.

Contributor

zaggino commented Mar 18, 2014

Btw, is there a reason Brackets is still using less 1.4.2 when latest version is 1.7.0 ? The fix works for both versions and I didn't find any ill effects when upgrading to 1.7.0.

@le717

This comment has been minimized.

Show comment
Hide comment
@le717

le717 Mar 18, 2014

Contributor

@zaggino Discussion about updating Less to 1.7.0 is in #6730.

Contributor

le717 commented Mar 18, 2014

@zaggino Discussion about updating Less to 1.7.0 is in #6730.

@redmunds redmunds self-assigned this Mar 18, 2014

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

Thanks @le717, I thought I've seen it somewhere but I wasn't sure. Anyway it's not included in this PR.

Contributor

zaggino commented Mar 18, 2014

Thanks @le717, I thought I've seen it somewhere but I wasn't sure. Anyway it's not included in this PR.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Mar 19, 2014

Contributor

There's an Extension Utils unit test failing:

should attach LESS style sheets

Error: Expected '/* basic.less */
#project-title {
  font-size: 66px;
}
' to be '/* basic.less */
#project-title {
  font-weight: 800;
}
#project-title {
  font-size: 66px;
}
'.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/ExtensionUtils-test.js:134:40)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12
Contributor

redmunds commented Mar 19, 2014

There's an Extension Utils unit test failing:

should attach LESS style sheets

Error: Expected '/* basic.less */
#project-title {
  font-size: 66px;
}
' to be '/* basic.less */
#project-title {
  font-weight: 800;
}
#project-title {
  font-size: 66px;
}
'.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/ExtensionUtils-test.js:134:40)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12
@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2014

Contributor

@redmunds It seems my issue only relates to absolute url's. Do we have any function like FileSystem.isAbsolutePath which checks if url starts with (http|https|file):// ?

Contributor

zaggino commented Mar 19, 2014

@redmunds It seems my issue only relates to absolute url's. Do we have any function like FileSystem.isAbsolutePath which checks if url starts with (http|https|file):// ?

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2014

Contributor

I'd probably put the isAbsoluteUrl function into StringUtils.js and use that instead of file:// if there's not a better way.

Contributor

zaggino commented Mar 19, 2014

I'd probably put the isAbsoluteUrl function into StringUtils.js and use that instead of file:// if there's not a better way.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Mar 19, 2014

Contributor

In thirdparty/path-utils/path-utils.js there's an isAbsoluteUrl function.

Contributor

redmunds commented Mar 19, 2014

In thirdparty/path-utils/path-utils.js there's an isAbsoluteUrl function.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2014

Contributor

Review again please @redmunds

Contributor

zaggino commented Mar 19, 2014

Review again please @redmunds

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Mar 19, 2014

Contributor

Looks good. Just 1 minor comment.

Contributor

redmunds commented Mar 19, 2014

Looks good. Just 1 minor comment.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 19, 2014

Contributor

Done, comments removed.

Contributor

zaggino commented Mar 19, 2014

Done, comments removed.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Mar 19, 2014

Contributor

Looks good. Thanks for unit test! Merging.

Contributor

redmunds commented Mar 19, 2014

Looks good. Thanks for unit test! Merging.

redmunds added a commit that referenced this pull request Mar 19, 2014

Merge pull request #7230 from adobe/zaggino/less-fix
Fix a case when extension less file is importing other less files

@redmunds redmunds merged commit 0e8b080 into master Mar 19, 2014

1 check passed

default The Travis CI build passed
Details

@redmunds redmunds deleted the zaggino/less-fix branch Mar 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment