Issue #1883 + Typescript Plugin Improvements #1986

Charlesnorris509 · 2024-11-05T23:44:25Z

Key changes include using arrow functions and template literals for cleaner syntax, and optional chaining to streamline property access where values may be undefined. Destructured assignments enhance readability, and constants are used for immutability where applicable. Additionally, inline comments clarify each change for easy reference, while error handling improvements ensure the code remains robust and clear. These changes collectively modernize the codebase while preserving its original functionality and use cases

Enhance Readability and Documentation, Type Aliases: Introduced type aliases. Increased Type Safety: Refined types for better accuracy, especially in callback signatures and function definitions.

The improved code includes updates for readability, maintainability, and modern JavaScript practices. Key changes include using arrow functions and template literals for cleaner syntax, and optional chaining to streamline property access where values may be undefined. Destructured assignments enhance readability, and constants are used for immutability where applicable. Additionally, inline comments clarify each change for easy reference, while error handling improvements ensure the code remains robust and clear. These changes collectively modernize the codebase while preserving its original functionality and use cases.

Spread Syntax in Function Arguments: Used (...args) to collect arguments in stdfn.SPRINTF for better readability and flexibility. Template Literals and Arrow Functions: Simplified type-specific formatter functions with arrow functions for consistency and clarity.

mathiasrw · 2024-12-01T02:06:38Z

I love it. Thank you!

mathiasrw

Please see comments.

mathiasrw · 2024-12-01T02:24:19Z

src/56sprintf.js

 	});
 };

+// Regular expression updated with comments for readability


That comments?

mathiasrw · 2024-12-01T02:29:35Z

src/56sprintf.js

+
+stdfn.SPRINTF.H = (ins, x) => {
+	let cleanIns = String(ins).replace(/,/g, ''); // Remove commas for number parsing
+	x[2] = x[2] === '-' ? '+' : '-'; // Adjust alignment for string output


Are you sure that comment is correct?

mathiasrw · 2024-12-01T02:31:27Z

src/63createvertex.js

@@ -1,21 +1,18 @@
 /*
 //
 // CREATE VERTEX for AlaSQL
-// Date: 21.04.2015
-// (c) 2015, Andrey Gershun
+// Date: 04/11/2024


Please do not change the date.

mathiasrw · 2024-12-01T02:32:06Z

src/63createvertex.js

-// Date: 21.04.2015
-// (c) 2015, Andrey Gershun
+// Date: 04/11/2024
+// (c) 2015, Andrey Gershu


Please dont change the surname of the author...

mathiasrw · 2024-12-01T02:32:31Z

src/63createvertex.js

-yy.CreateVertex = function (params) {
-	return Object.assign(this, params);
-};
+yy.CreateVertex = (params) => Object.assign(this, params); // Updated to arrow function


Please dont leave comments like this as it will not make sense in the future

mathiasrw · 2024-12-01T09:45:14Z

src/63createvertex.js

-	// todo: SET
-	// todo: CONTENT
-	// todo: SELECT
+	// Future TODOs: SET, CONTENT, SELECT


Please keep the original comments please

mathiasrw · 2024-12-01T09:45:30Z

src/63createvertex.js

-	var s = 'this.queriesfn[' + (this.queriesidx - 1) + '](this.params,null,' + context + ')';
-	return s;
+yy.CreateEdge.prototype.toJS = (context) => {
+	return `this.queriesfn[${this.queriesidx - 1}](this.params,null,${context})`; // Template literal for consistency


Please remove comment

mathiasrw · 2024-12-01T09:47:33Z

src/63createvertex.js

-
-		// Optional functions
-		namefn?.(edge);
+		namefn?.(edge); // Optional chaining


Please remove comment

mathiasrw · 2024-12-01T09:48:15Z

src/63createvertex.js

@@ -196,7 +157,7 @@ yy.CreateGraph.prototype.execute = function (databaseid, params, cb) {
 			if (g.class !== undefined) e.$class = g.class;

 			let db = alasql.databases[databaseid];
-			e.$id = e.$id !== undefined ? e.$id : db.counter++;
+			e.$id = e.$id ?? db.counter++;


mathiasrw · 2024-12-01T09:52:10Z

src/63createvertex.js

-			}
-		}
-		return undefined;
+		return Object.values(alasql.databases[alasql.useid].objects).find((obj) => obj.name === name);


We simply cant be making copies of all the data in an object for operations like this. We need to keep the original or find a way where we can search the original object without making a copy.

mathiasrw

Please see comments.

mathiasrw

We need comments to be related to what the code does (if needed at all) - not why something was changed.

mathiasrw · 2024-12-01T09:58:19Z

src/63createvertex.js

-		if (typeof g.json !== 'undefined') {
-			extend(v, new Function('params,alasql', 'var y;return ' + g.json.toJS())(params, alasql));
+		const db = alasql.databases[databaseid];
+		const v = {


Too many changes. And I dont feel like this will provide the same result. where. is the .prop aspect?

mathiasrw · 2024-12-01T10:00:38Z

src/63createvertex.js

 		res.push(v.$id);
 		return v;
 	}
 };
-yy.CreateGraph.prototype.compile1 = function (databaseid) {


Did we just remove all of this?!?

mathiasrw · 2024-12-01T10:02:26Z

src/76usedatabase.js

-yy.CreateDatabase = function (params) {
-	return Object.assign(this, params);
-};
+yy.CreateDatabase = (params) => Object.assign(this, params); // Updated to arrow function


Please keep regular functions for high level so they remain informative in the stack trace

mathiasrw · 2024-12-01T10:02:50Z

src/76usedatabase.js

 yy.CreateDatabase.prototype.toString = function () {
-	let s = 'CREATE '; // Ensure there's a space after CREATE
+	let s = 'CREATE '; // Added space after 'CREATE' for clarity


Please revert comment

mathiasrw · 2024-12-01T10:03:07Z

src/76usedatabase.js

 	if (this.engineid) s += `${this.engineid} `;
 	s += 'DATABASE ';
 	if (this.ifnotexists) s += 'IF NOT EXISTS ';
 	s += `${this.databaseid} `;

-	if (this.args && this.args.length > 0) {
+	if (this.args?.length) { // Optional chaining for args presence check


Please remove comment

mathiasrw · 2024-12-01T10:15:26Z

types/alasql.d.ts

-		 *
-		 * @type {tableLookUp}
-		 * @memberof database
+		 * Collection of tables in the database.


Why was this comment updated?

mathiasrw · 2024-12-01T10:15:47Z

types/alasql.d.ts

-		 *
-		 * @type {any[]}
-		 * @memberof table
+		 * Array of data rows within the table.


No value in the old comment?

mathiasrw · 2024-12-01T10:16:14Z

types/alasql.d.ts

-		 *
-		 * @type {databaseLookUp}
-		 * @memberof AlaSQL
+		 * Dictionary of databases managed by AlaSQL.


Why not the type notation?

mathiasrw · 2024-12-01T10:16:29Z

types/alasql.d.ts

-		 * Equivalent to alasql('USE '+databaseid). This will change the current
-		 * database to the one specified. This will update the useid property and
-		 * the tables property.
+		 * Switches the current database context.


Please keep the context of the old comment

mathiasrw · 2024-12-01T10:16:57Z

types/alasql.d.ts

-		 *
-		 * @type {string}
-		 * @memberof AlaSQL
+		 * Current active database ID. Defaults to 'alasql' if none specified.


Why remove the type notation

mathiasrw · 2024-12-01T10:21:57Z

Looks like the tests are not running. Please make sure to do a yarn format and then yarn test locally before you make a PR

mathiasrw · 2024-12-15T03:45:44Z

@Charlesnorris509 ?

Charlesnorris509 added 4 commits November 5, 2024 18:24

Update alasql.d.ts

d6c15ac

Enhance Readability and Documentation, Type Aliases: Introduced type aliases. Increased Type Safety: Refined types for better accuracy, especially in callback signatures and function definitions.

Update 56sprintf.js

914a969

Spread Syntax in Function Arguments: Used (...args) to collect arguments in stdfn.SPRINTF for better readability and flexibility. Template Literals and Arrow Functions: Simplified type-specific formatter functions with arrow functions for consistency and clarity.

Update 76usedatabase.js

01e3263

mathiasrw requested changes Dec 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1883 + Typescript Plugin Improvements #1986

Issue #1883 + Typescript Plugin Improvements #1986

Charlesnorris509 commented Nov 5, 2024

mathiasrw commented Dec 1, 2024

mathiasrw left a comment

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw left a comment

mathiasrw left a comment

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw Dec 1, 2024

mathiasrw commented Dec 1, 2024

mathiasrw commented Dec 15, 2024

Issue #1883 + Typescript Plugin Improvements #1986

Are you sure you want to change the base?

Issue #1883 + Typescript Plugin Improvements #1986

Conversation

Charlesnorris509 commented Nov 5, 2024

mathiasrw commented Dec 1, 2024

mathiasrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiasrw left a comment

Choose a reason for hiding this comment

mathiasrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiasrw commented Dec 1, 2024

mathiasrw commented Dec 15, 2024