Skip to content

Clean Makefile #189

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
wants to merge 2 commits into from
Closed

Clean Makefile #189

wants to merge 2 commits into from

Conversation

rpellerin
Copy link

  • Get rid of all the coffee files and keep generated JavaScript files
  • Make the appropriate changes to Makefile
  • Fix the Assertion failed that occurred with the message Assertion failed: you need to wait for the runtime to be ready (e.g. wait for main() to be called)
  • Fix Cannot compile sql.js emscripten-core/emscripten#4990

@dinedal
Copy link
Collaborator

dinedal commented Mar 2, 2017

Seems like it's failing in travis ci?

@rpellerin
Copy link
Author

Yes sorry, there's a variable scope problem. I'll fix this tomorrow morning.

@rpellerin
Copy link
Author

Currently SQL isn't exposed directly, only through Module. I'll restore the old behavior tomorrow, i.e. make it exposed globally.

@lovasoa
Copy link
Member

lovasoa commented Mar 2, 2017

Switching from coffeescript to JavaScript if not necessarily a bad idea, but if you want to do so, then you have to rewrite the API code as idiomatic js, you can't leave the generated files like that

@rpellerin
Copy link
Author

From https://travis-ci.org/kripken/sql.js/builds/207117119, one test passes, on Node 0.10, the others fail at Reading Blob.

@lovasoa
Copy link
Member

lovasoa commented Mar 2, 2017

Travis also indicates a warning message in the tests

writeStringToMemory is deprecated and should not be called! Use stringToUTF8() instead!

Copy link
Member

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The js code has to be rewritten in a readable form.
Not grouping var statements at the beginning of the methods would be nice.
Automatic documentation generation has to be taken care of (doc comments currently use codo)

js/api.js Outdated

Module['onRuntimeInitialized'] = function() {
// Generated by CoffeeScript 1.12.4
console.log('ok')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add a console.log ? Libraries should have undocumented side-effects, like writing to the console.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omission, sorry. Removed in latest commit.

size = sqlite3_column_bytes(this.stmt, pos);
ptr = sqlite3_column_blob(this.stmt, pos);
result = new Uint8Array(size);
for (i = k = 0, ref = size; 0 <= ref ? k < ref : k > ref; i = 0 <= ref ? ++k : --k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unreadable.

this['bind'](params) && this['step']();
}
results1 = [];
for (field = k = 0, ref = sqlite3_data_count(this.stmt); 0 <= ref ? k < ref : k > ref; field = 0 <= ref ? ++k : --k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unreadable.

Statement.prototype['getColumnNames'] = function() {
var i, k, ref, results1;
results1 = [];
for (i = k = 0, ref = sqlite3_data_count(this.stmt); 0 <= ref ? k < ref : k > ref; i = 0 <= ref ? ++k : --k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unreadable.


Statement.prototype.bindFromArray = function(values) {
var k, len, num, value;
for (num = k = 0, len = values.length; k < len; num = ++k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not idiomatic.

values = this['get'](params);
names = this['getColumnNames']();
rowObject = {};
for (i = k = 0, len = names.length; k < len; i = ++k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not idiomatic.

Database.prototype['export'] = function() {
var _, binaryDb, ref, stmt;
ref = this.statements;
for (_ in ref) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not idiomatic.

Database.prototype['close'] = function() {
var _, ref, stmt;
ref = this.statements;
for (_ in ref) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not idiomatic.

wrapped_func = function(cx, argc, argv) {
var arg, args, data_func, i, k, ref, result, value_ptr, value_type;
args = [];
for (i = k = 0, ref = argc; 0 <= ref ? k <= ref : k >= ref; i = 0 <= ref ? ++k : --k) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unreadable.

size = sqlite3_value_bytes(ptr);
blob_ptr = sqlite3_value_blob(ptr);
blob_arg = new Uint8Array(size);
for (j = l = 0, ref1 = size; 0 <= ref1 ? l < ref1 : l > ref1; j = 0 <= ref1 ? ++l : --l) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unreadable

@bhollis
Copy link

bhollis commented Apr 2, 2017

@rpellerin I'm guessing this would be easier to merge if all the different things you're fixing were separate PRs.

@femski
Copy link

femski commented May 19, 2017

Any update ? Can this be merged ?

@dinedal
Copy link
Collaborator

dinedal commented May 22, 2017

Any update ? Can this be merged ?

The travis tests are failing still, either they need to be updated or they need to pass as is.

@kaizhu256
Copy link
Member

can this pull-request be closed as a duplicate of #349 ?

@lovasoa lovasoa closed this Mar 1, 2020
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.

Cannot compile sql.js
6 participants