Commit 47d6386d by Adeel

API: Provides support for array of importers.

* In order to skip the importer, user must
  return `sass.types.NULL` (or the shorter
  alias `sass.NULL`).
* See the added test on usage.
* Backward compatible: part of me want still
  wants to make it non-backward compatible
  because:
  * We can: in major version v3.0.
  * It will keep the API clean.
  * It will keep the docs clear: type of
    `options.importers` is array of functions.
* Updates docs.

Issue URL: #821.
PR URL: #832.
parent edcf76ff
...@@ -57,21 +57,21 @@ var result = sass.renderSync({ ...@@ -57,21 +57,21 @@ var result = sass.renderSync({
## Options ## Options
### file ### file
Type: `String | null` Type: `String`
Default: `null` Default: `null`
**Special**: `file` or `data` must be specified **Special**: `file` or `data` must be specified
Path to a file for [libsass] to render. Path to a file for [libsass] to render.
### data ### data
Type: `String | null` Type: `String`
Default: `null` Default: `null`
**Special**: `file` or `data` must be specified **Special**: `file` or `data` must be specified
A string to pass to [libsass] to render. It is recommended that you use `includePaths` in conjunction with this so that [libsass] can find files when using the `@import` directive. A string to pass to [libsass] to render. It is recommended that you use `includePaths` in conjunction with this so that [libsass] can find files when using the `@import` directive.
### importer (>= v2.0.0) ### importer (>= v2.0.0)
Type: `Function` signature `function(url, prev, done)` Type: `Function | Function[]` signature `function(url, prev, done)`
Default: `undefined` Default: `undefined`
Function Parameters and Information: Function Parameters and Information:
...@@ -89,6 +89,8 @@ When returning or calling `done()` with `{ contents: "String" }`, the string val ...@@ -89,6 +89,8 @@ When returning or calling `done()` with `{ contents: "String" }`, the string val
Starting from v3.0.0, `this` refers to a contextual scope for the immediate run of `sass.render` or `sass.renderSync` Starting from v3.0.0, `this` refers to a contextual scope for the immediate run of `sass.render` or `sass.renderSync`
Starting from v3.0.0, importer can be an array of functions, which will be called by libsass in the order of their occurance in array. This helps user specify special importer for particular kind of path (filesystem, http). If the importer does not handle particular path, it should return `sass.NULL`. See [functions section](#functions) for more details on Sass types.
### functions (>= v3.0.0) ### functions (>= v3.0.0)
`functions` is an `Object` that holds a collection of custom functions that may be invoked by the sass files being compiled. They may take zero or more input parameters and must return a value either synchronously (`return ...;`) or asynchronously (`done();`). Those parameters will be instances of one of the constructors contained in the `require('node-sass').types` hash. The return value must be of one of these types as well. See the list of available types below: `functions` is an `Object` that holds a collection of custom functions that may be invoked by the sass files being compiled. They may take zero or more input parameters and must return a value either synchronously (`return ...;`) or asynchronously (`done();`). Those parameters will be instances of one of the constructors contained in the `require('node-sass').types` hash. The return value must be of one of these types as well. See the list of available types below:
......
...@@ -282,17 +282,33 @@ module.exports.render = function(options, cb) { ...@@ -282,17 +282,33 @@ module.exports.render = function(options, cb) {
var importer = options.importer; var importer = options.importer;
if (importer) { if (importer) {
options.importer = function(file, prev, bridge) { if (Array.isArray(importer)) {
function done(data) { importer.forEach(function(subject, index) {
bridge.success(data); options.importer[index] = function(file, prev, bridge) {
} function done(data) {
bridge.success(data);
}
var result = subject.call(options.context, file, prev, done);
if (result) {
done(result === module.exports.NULL ? null : result);
}
};
});
} else {
options.importer = function(file, prev, bridge) {
function done(data) {
bridge.success(data);
}
var result = importer.call(options.context, file, prev, done); var result = importer.call(options.context, file, prev, done);
if (result) { if (result) {
done(result); done(result === module.exports.NULL ? null : result);
} }
}; };
}
} }
var functions = options.functions; var functions = options.functions;
...@@ -300,8 +316,8 @@ module.exports.render = function(options, cb) { ...@@ -300,8 +316,8 @@ module.exports.render = function(options, cb) {
if (functions) { if (functions) {
options.functions = {}; options.functions = {};
Object.keys(functions).forEach(function(signature) { Object.keys(functions).forEach(function(subject) {
var cb = normalizeFunctionSignature(signature, functions[signature]); var cb = normalizeFunctionSignature(subject, functions[subject]);
options.functions[cb.signature] = function() { options.functions[cb.signature] = function() {
var args = Array.prototype.slice.call(arguments), var args = Array.prototype.slice.call(arguments),
...@@ -336,9 +352,21 @@ module.exports.renderSync = function(options) { ...@@ -336,9 +352,21 @@ module.exports.renderSync = function(options) {
var importer = options.importer; var importer = options.importer;
if (importer) { if (importer) {
options.importer = function(file, prev) { if (Array.isArray(importer)) {
return importer.call(options.context, file, prev); importer.forEach(function(subject, index) {
}; options.importer[index] = function(file, prev) {
var result = subject.call(options.context, file, prev);
return result === module.exports.NULL ? null : result;
};
});
} else {
options.importer = function(file, prev) {
var result = importer.call(options.context, file, prev);
return result === module.exports.NULL ? null : result;
};
}
} }
var functions = options.functions; var functions = options.functions;
......
...@@ -5,20 +5,24 @@ ...@@ -5,20 +5,24 @@
#include "create_string.h" #include "create_string.h"
#include "sass_types/factory.h" #include "sass_types/factory.h"
struct Sass_Import** sass_importer(const char* file, const char* prev, void* cookie) Sass_Import_List sass_importer(const char* cur_path, Sass_Importer_Entry cb, struct Sass_Compiler* comp)
{ {
void* cookie = sass_importer_get_cookie(cb);
struct Sass_Import* previous = sass_compiler_get_last_import(comp);
const char* prev_path = sass_import_get_path(previous);
sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(cookie); sass_context_wrapper* ctx_w = static_cast<sass_context_wrapper*>(cookie);
CustomImporterBridge& bridge = *(ctx_w->importer_bridge); CustomImporterBridge& bridge = *(static_cast<CustomImporterBridge*>(cookie));
std::vector<void*> argv; std::vector<void*> argv;
argv.push_back((void*)file); argv.push_back((void*)cur_path);
argv.push_back((void*)prev); argv.push_back((void*)prev_path);
return bridge(argv); return bridge(argv);
} }
union Sass_Value* sass_custom_function(const union Sass_Value* s_args, void* cookie) union Sass_Value* sass_custom_function(const union Sass_Value* s_args, Sass_Function_Entry cb, struct Sass_Options* opts)
{ {
void* cookie = sass_function_get_cookie(cb);
CustomFunctionBridge& bridge = *(static_cast<CustomFunctionBridge*>(cookie)); CustomFunctionBridge& bridge = *(static_cast<CustomFunctionBridge*>(cookie));
std::vector<void*> argv; std::vector<void*> argv;
...@@ -65,13 +69,6 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx ...@@ -65,13 +69,6 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx
ctx_w->error_callback = new NanCallback(error_callback); ctx_w->error_callback = new NanCallback(error_callback);
} }
Local<Function> importer_callback = Local<Function>::Cast(options->Get(NanNew("importer")));
if (importer_callback->IsFunction()) {
ctx_w->importer_bridge = new CustomImporterBridge(new NanCallback(importer_callback), ctx_w->is_sync);
sass_option_set_importer(sass_options, sass_make_importer(sass_importer, ctx_w));
}
if (!is_file) { if (!is_file) {
ctx_w->file = create_string(options->Get(NanNew("file"))); ctx_w->file = create_string(options->Get(NanNew("file")));
sass_option_set_input_path(sass_options, ctx_w->file); sass_option_set_input_path(sass_options, ctx_w->file);
...@@ -106,25 +103,58 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx ...@@ -106,25 +103,58 @@ void ExtractOptions(Local<Object> options, void* cptr, sass_context_wrapper* ctx
sass_option_set_indent(sass_options, ctx_w->indent); sass_option_set_indent(sass_options, ctx_w->indent);
sass_option_set_linefeed(sass_options, ctx_w->linefeed); sass_option_set_linefeed(sass_options, ctx_w->linefeed);
Local<Object> custom_functions = Local<Object>::Cast(options->Get(NanNew("functions"))); Local<Value> importer_callback = options->Get(NanNew("importer"));
if (importer_callback->IsFunction()) {
Local<Function> importer = Local<Function>::Cast(importer_callback);
auto bridge = std::make_shared<CustomImporterBridge>(new NanCallback(importer), ctx_w->is_sync);
ctx_w->importer_bridges.push_back(bridge);
Sass_Importer_List c_importers = sass_make_importer_list(1);
c_importers[0] = sass_make_importer(sass_importer, 0, bridge.get());
sass_option_set_c_importers(sass_options, c_importers);
}
else if (importer_callback->IsArray()) {
Handle<Array> importers = Handle<Array>::Cast(importer_callback);
Sass_Importer_List c_importers = sass_make_importer_list(importers->Length());
for (size_t i = 0; i < importers->Length(); ++i) {
Local<Function> callback = Local<Function>::Cast(importers->Get(static_cast<uint32_t>(i)));
if (!callback->IsFunction()) {
NanThrowError(NanNew("options.importer must be set to a function or array of functions"));
}
auto bridge = std::make_shared<CustomImporterBridge>(new NanCallback(callback), ctx_w->is_sync);
ctx_w->importer_bridges.push_back(bridge);
c_importers[i] = sass_make_importer(sass_importer, importers->Length() - i - 1, bridge.get());
}
sass_option_set_c_importers(sass_options, c_importers);
}
Local<Value> custom_functions = options->Get(NanNew("functions"));
if (custom_functions->IsObject()) { if (custom_functions->IsObject()) {
Local<Array> signatures = custom_functions->GetOwnPropertyNames(); Local<Object> functions = Local<Object>::Cast(custom_functions);
Local<Array> signatures = functions->GetOwnPropertyNames();
unsigned num_signatures = signatures->Length(); unsigned num_signatures = signatures->Length();
Sass_C_Function_List fn_list = sass_make_function_list(num_signatures); Sass_Function_List fn_list = sass_make_function_list(num_signatures);
for (unsigned i = 0; i < num_signatures; i++) { for (unsigned i = 0; i < num_signatures; i++) {
Local<String> signature = Local<String>::Cast(signatures->Get(NanNew(i))); Local<String> signature = Local<String>::Cast(signatures->Get(NanNew(i)));
Local<Function> callback = Local<Function>::Cast(custom_functions->Get(signature)); Local<Function> callback = Local<Function>::Cast(functions->Get(signature));
if (!signature->IsString() || !callback->IsFunction()) { if (!signature->IsString() || !callback->IsFunction()) {
NanThrowError(NanNew("options.functions must be a (signature -> function) hash")); NanThrowError(NanNew("options.functions must be a (signature -> function) hash"));
} }
CustomFunctionBridge* bridge = new CustomFunctionBridge(new NanCallback(callback), ctx_w->is_sync); auto bridge = std::make_shared<CustomFunctionBridge>(new NanCallback(callback), ctx_w->is_sync);
ctx_w->function_bridges.push_back(bridge); ctx_w->function_bridges.push_back(bridge);
Sass_C_Function_Callback fn = sass_make_function(create_string(signature), sass_custom_function, bridge); Sass_Function_Entry fn = sass_make_function(create_string(signature), sass_custom_function, bridge.get());
sass_function_set_list_entry(fn_list, i, fn); sass_function_set_list_entry(fn_list, i, fn);
} }
...@@ -274,7 +304,8 @@ NAN_METHOD(render_file_sync) { ...@@ -274,7 +304,8 @@ NAN_METHOD(render_file_sync) {
int result = GetResult(ctx_w, ctx, true); int result = GetResult(ctx_w, ctx, true);
sass_wrapper_dispose(ctx_w, input_path); free(input_path);
sass_free_context_wrapper(ctx_w);
NanReturnValue(NanNew<Boolean>(result == 0)); NanReturnValue(NanNew<Boolean>(result == 0));
} }
......
...@@ -5,7 +5,8 @@ ...@@ -5,7 +5,8 @@
Sass_Value* CustomFunctionBridge::post_process_return_value(Handle<Value> val) const { Sass_Value* CustomFunctionBridge::post_process_return_value(Handle<Value> val) const {
try { try {
return SassTypes::Factory::unwrap(val)->get_sass_value(); return SassTypes::Factory::unwrap(val)->get_sass_value();
} catch (const std::invalid_argument& e) { }
catch (const std::invalid_argument& e) {
return sass_make_error(e.what()); return sass_make_error(e.what());
} }
} }
...@@ -14,9 +15,7 @@ std::vector<Handle<Value>> CustomFunctionBridge::pre_process_args(std::vector<vo ...@@ -14,9 +15,7 @@ std::vector<Handle<Value>> CustomFunctionBridge::pre_process_args(std::vector<vo
std::vector<Handle<Value>> argv = std::vector<Handle<Value>>(); std::vector<Handle<Value>> argv = std::vector<Handle<Value>>();
for (void* value : in) { for (void* value : in) {
argv.push_back( argv.push_back(SassTypes::Factory::create(static_cast<Sass_Value*>(value))->get_js_object());
SassTypes::Factory::create(static_cast<Sass_Value*>(value))->get_js_object()
);
} }
return argv; return argv;
......
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
#include "create_string.h" #include "create_string.h"
SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val) const { SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val) const {
SassImportList imports; SassImportList imports = 0;
NanScope(); NanScope();
Local<Value> returned_value = NanNew(val); Local<Value> returned_value = NanNew(val);
...@@ -32,8 +32,9 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val ...@@ -32,8 +32,9 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val
else { else {
char* path = create_string(object->Get(NanNew<String>("file"))); char* path = create_string(object->Get(NanNew<String>("file")));
char* contents = create_string(object->Get(NanNew<String>("contents"))); char* contents = create_string(object->Get(NanNew<String>("contents")));
char* srcmap = create_string(object->Get(NanNew<String>("map")));
imports[i] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0); imports[i] = sass_make_import_entry(path, contents, srcmap);
} }
} }
} }
...@@ -51,12 +52,9 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val ...@@ -51,12 +52,9 @@ SassImportList CustomImporterBridge::post_process_return_value(Handle<Value> val
Local<Object> object = Local<Object>::Cast(returned_value); Local<Object> object = Local<Object>::Cast(returned_value);
char* path = create_string(object->Get(NanNew<String>("file"))); char* path = create_string(object->Get(NanNew<String>("file")));
char* contents = create_string(object->Get(NanNew<String>("contents"))); char* contents = create_string(object->Get(NanNew<String>("contents")));
char* srcmap = create_string(object->Get(NanNew<String>("map")));
imports[0] = sass_make_import_entry(path, (!contents || contents[0] == '\0') ? 0 : strdup(contents), 0); imports[0] = sass_make_import_entry(path, contents, srcmap);
}
else {
imports = sass_make_import_list(1);
imports[0] = sass_make_import_entry((char const*) this->argv[0], 0, 0);
} }
return imports; return imports;
......
...@@ -7,7 +7,7 @@ ...@@ -7,7 +7,7 @@
using namespace v8; using namespace v8;
typedef Sass_Import** SassImportList; typedef Sass_Import_List SassImportList;
class CustomImporterBridge : public CallbackBridge<SassImportList> { class CustomImporterBridge : public CallbackBridge<SassImportList> {
public: public:
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
'libsass/constants.cpp', 'libsass/constants.cpp',
'libsass/context.cpp', 'libsass/context.cpp',
'libsass/contextualize.cpp', 'libsass/contextualize.cpp',
'libsass/contextualize_eval.cpp',
'libsass/cssize.cpp', 'libsass/cssize.cpp',
'libsass/emitter.cpp', 'libsass/emitter.cpp',
'libsass/error_handling.cpp', 'libsass/error_handling.cpp',
...@@ -21,6 +22,8 @@ ...@@ -21,6 +22,8 @@
'libsass/functions.cpp', 'libsass/functions.cpp',
'libsass/inspect.cpp', 'libsass/inspect.cpp',
'libsass/json.cpp', 'libsass/json.cpp',
'libsass/lexer.cpp',
'libsass/listize.cpp',
'libsass/node.cpp', 'libsass/node.cpp',
'libsass/output.cpp', 'libsass/output.cpp',
'libsass/parser.cpp', 'libsass/parser.cpp',
...@@ -72,7 +75,7 @@ ...@@ -72,7 +75,7 @@
'VCCLCompilerTool': { 'VCCLCompilerTool': {
'AdditionalOptions': [ 'AdditionalOptions': [
'/GR', '/GR',
'/EHsc' '/EHs'
] ]
} }
} }
......
...@@ -26,7 +26,7 @@ extern "C" { ...@@ -26,7 +26,7 @@ extern "C" {
return (sass_context_wrapper*)calloc(1, sizeof(sass_context_wrapper)); return (sass_context_wrapper*)calloc(1, sizeof(sass_context_wrapper));
} }
void sass_wrapper_dispose(struct sass_context_wrapper* ctx_w, char* string = 0) { void sass_free_context_wrapper(sass_context_wrapper* ctx_w) {
if (ctx_w->dctx) { if (ctx_w->dctx) {
sass_delete_data_context(ctx_w->dctx); sass_delete_data_context(ctx_w->dctx);
} }
...@@ -46,23 +46,8 @@ extern "C" { ...@@ -46,23 +46,8 @@ extern "C" {
free(ctx_w->source_map_root); free(ctx_w->source_map_root);
free(ctx_w->indent); free(ctx_w->indent);
if (string) { ctx_w->importer_bridges.resize(0);
free(string); ctx_w->function_bridges.resize(0);
}
if (!ctx_w->function_bridges.empty()) {
for (CustomFunctionBridge* bridge : ctx_w->function_bridges) {
delete bridge;
}
}
if (ctx_w->importer_bridge) {
delete ctx_w->importer_bridge;
}
}
void sass_free_context_wrapper(sass_context_wrapper* ctx_w) {
sass_wrapper_dispose(ctx_w);
free(ctx_w); free(ctx_w);
} }
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
#define SASS_CONTEXT_WRAPPER #define SASS_CONTEXT_WRAPPER
#include <vector> #include <vector>
#include <memory>
#include <nan.h> #include <nan.h>
#include <stdlib.h> #include <stdlib.h>
#include <condition_variable> #include <condition_variable>
...@@ -44,12 +45,11 @@ extern "C" { ...@@ -44,12 +45,11 @@ extern "C" {
NanCallback* error_callback; NanCallback* error_callback;
NanCallback* success_callback; NanCallback* success_callback;
std::vector<CustomFunctionBridge*> function_bridges; std::vector<std::shared_ptr<CustomFunctionBridge>> function_bridges;
CustomImporterBridge* importer_bridge; std::vector<std::shared_ptr<CustomImporterBridge>> importer_bridges;
}; };
struct sass_context_wrapper* sass_make_context_wrapper(void); struct sass_context_wrapper* sass_make_context_wrapper(void);
void sass_wrapper_dispose(struct sass_context_wrapper*, char*);
void sass_free_context_wrapper(struct sass_context_wrapper*); void sass_free_context_wrapper(struct sass_context_wrapper*);
#ifdef __cplusplus #ifdef __cplusplus
......
...@@ -341,6 +341,25 @@ describe('api', function() { ...@@ -341,6 +341,25 @@ describe('api', function() {
}); });
}); });
it('should accept arrays of importers and return respect the order', function(done) {
sass.render({
file: fixture('include-files/index.scss'),
importer: [
function() {
return sass.NULL;
},
function() {
return {
contents: 'div {color: yellow;}'
};
}
]
}, function(error, result) {
assert.equal(result.css.toString().trim(), 'div {\n color: yellow; }\n\ndiv {\n color: yellow; }');
done();
});
});
it('should be able to see its options in this.options', function(done) { it('should be able to see its options in this.options', function(done) {
var fxt = fixture('include-files/index.scss'); var fxt = fixture('include-files/index.scss');
sass.render({ sass.render({
...@@ -1146,6 +1165,25 @@ describe('api', function() { ...@@ -1146,6 +1165,25 @@ describe('api', function() {
done(); done();
}); });
it('should accept arrays of importers and return respect the order', function(done) {
var result = sass.renderSync({
file: fixture('include-files/index.scss'),
importer: [
function() {
return sass.NULL;
},
function() {
return {
contents: 'div {color: yellow;}'
};
}
]
});
assert.equal(result.css.toString().trim(), 'div {\n color: yellow; }\n\ndiv {\n color: yellow; }');
done();
});
it('should be able to see its options in this.options', function(done) { it('should be able to see its options in this.options', function(done) {
var fxt = fixture('include-files/index.scss'); var fxt = fixture('include-files/index.scss');
var sync = false; var sync = false;
...@@ -1161,7 +1199,6 @@ describe('api', function() { ...@@ -1161,7 +1199,6 @@ describe('api', function() {
done(); done();
}); });
it('should throw user-defined error', function(done) { it('should throw user-defined error', function(done) {
assert.throws(function() { assert.throws(function() {
sass.renderSync({ sass.renderSync({
......
...@@ -226,7 +226,7 @@ describe('cli', function() { ...@@ -226,7 +226,7 @@ describe('cli', function() {
}); });
setTimeout(function() { setTimeout(function() {
fs.appendFileSync(foo, 'body{background:white}'); fs.appendFileSync(foo, 'body{background:white}\n');
}, 500); }, 500);
}); });
}); });
...@@ -410,6 +410,19 @@ describe('cli', function() { ...@@ -410,6 +410,19 @@ describe('cli', function() {
}); });
}); });
it('should accept arrays of importers and return respect the order', function(done) {
var bin = spawn(cli, [
src, '--output', path.dirname(dest),
'--importer', fixture('extras/my_custom_arrays_of_importers.js')
]);
bin.once('close', function() {
assert.equal(read(dest, 'utf8').trim(), expected);
fs.unlinkSync(dest);
done();
});
});
it('should return error for invalid importer file path', function(done) { it('should return error for invalid importer file path', function(done) {
var bin = spawn(cli, [ var bin = spawn(cli, [
src, '--output', path.dirname(dest), src, '--output', path.dirname(dest),
......
var sass = require('../../..');
module.exports = [
function() {
return sass.NULL;
},
function() {
return {
contents: 'div {color: yellow;}'
};
}
];
...@@ -5,6 +5,6 @@ ...@@ -5,6 +5,6 @@
"index.scss" "index.scss"
], ],
"sourcesContent": [], "sourcesContent": [],
"mappings": "AAAA,OAAO,CAAC;EACN,KAAK,EAAE,GAAI;EACX,MAAM,EAAE,IAAI,GAFL;;AAKD,OAAO,CAAC,EAAE,CAAP;EACT,eAAe,EAAE,IAAK,GADZ;;AAIJ,OAAO,CAAC,EAAE,CAAP;EACT,KAAK,EAAE,IAAK,GADF;EAGV,OAAO,CAAC,EAAE,CAAC,CAAC,CAAV;IACA,WAAW,EAAE,IAAK,GADjB", "mappings": "AAAO,OAAO,CAAN;EACN,KAAK,EAAE,GAAI;EACX,MAAM,EAAE,IAAI,GAFL;;AAKC,OAAO,CAAC,EAAE,CAAT;EACT,eAAe,EAAE,IAAK,GADZ;;AAIF,OAAO,CAAC,EAAE,CAAT;EACT,KAAK,EAAE,IAAK,GADF;EAGT,OAAO,CAAC,EAAE,CAAC,CAAC,CAAX;IACA,WAAW,EAAE,IAAK,GADjB",
"names": [] "names": []
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment