TooTallNate / node-lame

Node.js native bindings to libmp3lame & libmpg123
MIT License
567 stars 113 forks source link

Added bindings for mp3 info method (#31 refactor) #82

Closed dmednis closed 3 years ago

dmednis commented 6 years ago

Refactored PR #31 to be mergable.

LinusU commented 6 years ago

This looks good, the only thing I would like to change is the enums (version, mode, flags and vbr). Could we either just remove them from the info object (and add them in a later version) or turn them into strings instead, so that they are usable from node land.

switch (frameinfo.version) {
  case MPG123_1_0: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("MPG123_1_0")); break;
  case MPG123_2_0: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("MPG123_2_0")); break;
  case MPG123_2_5: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("MPG123_2_5")); break;
}

edit:

maybe even

switch (frameinfo.version) {
  case MPG123_1_0: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("1.0")); break;
  case MPG123_2_0: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("2.0")); break;
  case MPG123_2_5: Nan::Set(o, Nan::New<String>("version").ToLocalChecked(), Nan::New<String>("2.5")); break;
}