Origin/refactor/retype functions READY FOR REVIEW #391

Banshaan · 2024-04-25T13:39:06Z

All functions that utilize http.service have been assigned (hopefully) the right types.
I also added/revised some interfaces so that they could be (better) utilized.

…so added TODO comments for tracking progress

… added to list of interfaces.

…refrank/ladybug-frontend into origin/refactor/correct-type

Co-authored-by: Matthijs Smets <93487259+MatthijsSmets@users.noreply.github.com>

…some interfaces

…nterfaces to help accomplish that.

…d interfaces to help accomplish that.

utilize http.service functions.

…ll interfaces used to achieve type safety. only some issues left in table.component.html

…onent.html

philipsens

Could you first resolve the git conflicts?
That way we can have a better representation of what will get merged.

Matthbo

I see a lot of let keywords to initialize a variable.
Maybe its best to see if its really needed and change it to const when the variables aren't being reassigned, try to do this for every bit of code you've been working on so that hopefully at some point its correct in most places

src/app/debug/debug.component.ts

src/app/debug/display/display.component.ts

src/app/debug/table/table.component.ts

src/app/debug/debug-tree/debug-tree.component.ts

src/app/debug/table/table.component.ts

src/app/report/edit-display/edit-display.component.html

src/app/report/edit-display/edit-display.component.ts

src/app/shared/pipes/table-cell-shortener.pipe.ts

MatthijsSmets

Please take a look at my comments. In some cases it feels like you're solving as you go, instead of taking a step back and trying to work out a better solution.

MatthijsSmets · 2024-04-30T09:41:42Z

src/app/debug/table/table.component.ts

  }

  openSelected(): void {
    for (const report of this.tableSettings.reportMetadata) {
      if (report.checked) {
-        this.openReport(report.storageId);
+        this.openReport(`${report.storageId}`);


Maybe its better to set the parameter type of openReport to number? That way it doesn't have to be cast.

MatthijsSmets · 2024-04-30T09:43:12Z

src/app/debug/table/table.component.ts

+      (report: MetaData) => report.checked,
+    );
+    this.httpService
+      .getReport(`${reportTab?.storageId}`, this.viewSettings.currentView.storageName)


What if reportTab is undefined?

MatthijsSmets · 2024-04-30T09:43:51Z

src/app/debug/table/table.component.ts

  }

  compareTwoReports(): void {
-    let compareReports: any = {};
+    let compareReports: Partial<CompareReports> = {};


This can be a const

MatthijsSmets · 2024-04-30T09:49:43Z

src/app/report/edit-display/edit-display.component.html

-  <div *ngIf="report.preTruncatedMessageLength > 0" id="truncatedMessageLabel" class="btn btn-static my-2 mx-1 px-2">
-    Message is truncated ( {{ report.preTruncatedMessageLength - report.message.length}} characters removed)
+  <div *ngIf="report.preTruncatedMessageLength && report.preTruncatedMessageLength > 0" id="truncatedMessageLabel" class="btn btn-static my-2 mx-1 px-2">
+    Message is truncated ( {{ report.preTruncatedMessageLength - report.message!.length}} characters removed)


Right now you use the ! to tell typescript that this wont be null or undefined when accessed. However the report.message can be null as its not in the if condition.

This has been fixed

MatthijsSmets · 2024-04-30T09:52:53Z

src/app/report/edit-display/edit-display.component.ts

-    let reportId: string = this.report.storageId;
-    this.httpService.runDisplayReport(reportId, this.currentView.storageName).subscribe((response) => {
-      let element = document.querySelector('#showRerunResult')!;
+    const reportId: string = String(this.report.storageId);


I think the storageId should either be saved as a string in the model or always be used as a number. Right now the implementation makes it so it always has to be cast.

StorageId is now saved as a string in the model.

MatthijsSmets · 2024-04-30T11:28:06Z

src/app/shared/interfaces/metadata.ts

+  'CONVERSATION ID': string;
+  'CORRELATION ID': string;
+  'ENDPOINT NAME': string;
+  'NR OF CHECKPOINTS': string;


Using this hardcoded all caps string as an attribute name is not done. Please make them camelCase and maybe use a transform function to transform the backend response with all caps: "CONVERSION ID" => conversationId into this model.

This has been fixed

MatthijsSmets · 2024-04-30T11:31:05Z

src/app/shared/interfaces/metadata.ts

+  STATUS: string;
+  TIMESTAMP: string;
+
+  [key: string]: string | boolean;


I feel like this is not necessary for an object thats retrieved from the backend. This should only be used when you do not know what the object can contain.

This has been taken out.

MatthijsSmets · 2024-04-30T11:32:02Z

src/app/shared/pipes/table-cell-shortener.pipe.ts

@@ -7,10 +7,13 @@ import { Subscription } from 'rxjs';
  standalone: true,
 })
 export class TableCellShortenerPipe implements PipeTransform {
-  transform(value: string): string {
+  transform(value: string | boolean): string {


I'm not sure why this can be a boolean, as far as i know a table cell never has a boolean value.

This issue has been fixed, it had to do with the array it looped over also having a boolean value (checked). I changed the interface around a bit so that this wouldnt cause issues anymore

src/app/debug/debug-tree/debug-tree.component.ts

MatthijsSmets · 2024-04-30T11:36:47Z

src/app/debug/display/display.component.ts

+    metadataLabels: [],
+    defaultView: false,
+    crudStorage: false,
+    metadataNames: [],
+    nodeLinkStrategy: '',
+    storageName: '',


You're setting these attributes to an empty array or string. Is this necessary or should the model maybe changed so that the attributes can be undefined? I've seen you do this in other places as well.

The reason i did it this way is because if i make them able to be undefined in the interface, there'll be many errors complaining that the value could be undefined. Should i fix this by having those places first check if they have a value or is there a better way of doing this?

Yes it's better to check whether the value is (not) undefined than to set the value to an empty string or array.

This has been fixed, instead of setting the properties to empty arrays/strings it now checks whether they have a value or not.

src/app/debug/table/table.component.ts

philipsens · 2024-05-21T08:18:28Z

@Banshaan Is this almost ready?

Matthbo

The code looks good but I haven't tested the functionality, I'm assuming it's working as it should since this is supposed to be a refactor pr

src/app/debug/display/display.component.ts

Added suggestion to turn a string concatenation into a template string Co-authored-by: Vivy <4380412+Matthbo@users.noreply.github.com>

philipsens · 2024-05-24T12:27:00Z

Please request review when you're finished.

philipsens · 2024-05-24T12:43:37Z

src/app/debug/table/table.component.html

+        <button ngbDropdownItem (click)="downloadReports(true, true)"> XML & Binary </button>
+        <button ngbDropdownItem (click)="downloadReports(false, true)"> XML <i class="fa fa-info-circle" title="Only a binary file can be uploaded again"></i></button>
+        <button ngbDropdownItem (click)="downloadReports(true, false)"> Binary </button>


Where are the spaces for? :o

MatthijsSmets

Please take a look at my previous comment about taking a step back and deciding what you need to do instead of solving as you go.

MatthijsSmets · 2024-05-24T11:56:32Z

src/app/debug/debug-tree/debug-tree.component.ts

+    let currentViewName: string = '';
+    if (currentView.name) {
+      currentViewName = currentView.name;
+    }


Suggested change

let currentViewName: string = '';

if (currentView.name) {

currentViewName = currentView.name;

}

let currentViewName: string = currentView.name ?? '';

MatthijsSmets · 2024-05-24T12:00:06Z

src/app/debug/display/display.component.ts

  @Output() closeReportEvent = new EventEmitter<any>();
  @ViewChild('editorComponent') editorComponent!: CustomEditorComponent;
  @ViewChild(DisplayTableComponent)
  displayTableComponent!: DisplayTableComponent;
  metadataTableVisible: boolean = false;
  @ViewChild('container') container!: ElementRef;
  @Input() containerHeight!: number;
+  @Input() currentView: View = {};


If currentView is an input attribute, why is it initialised with an empty object?
Wouldn't it be better to do it like the other input?

Suggested change

@Input() currentView: View = {};

@Input() currentView!: View;

MatthijsSmets · 2024-05-24T12:03:16Z

src/app/debug/display/display.component.ts

-    this.helperService.download('id=' + queryString + '&', this.currentView.storageName, exportBinary, exportXML);
+    const queryString: string = this.report.xml ? this.report.storageId : this.report.uid.split('#')[0];
+    if (this.currentView.storageName) {
+      this.helperService.download('id=' + queryString + '&', this.currentView.storageName, exportBinary, exportXML);


Please make use of string templating instead of concatenation

Suggested change

this.helperService.download('id=' + queryString + '&', this.currentView.storageName, exportBinary, exportXML);

this.helperService.download(`id=${queryString}&`, this.currentView.storageName, exportBinary, exportXML);

MatthijsSmets · 2024-05-24T12:20:57Z

src/app/debug/display/display.component.ts

  copyReport(): void {
    const storageId: number = this.report.xml ? +this.report.storageId : +this.report.uid.split('#')[0];
-    const data: any = {};
-    data[this.currentView.storageName] = [storageId];
+    const data: Record<string, number[]> = {};
+    if (this.currentView.storageName) {
+      data[this.currentView.storageName] = [storageId];
+    }
    this.httpService.copyReport(data, 'Test').subscribe(); // TODO: storage is hardcoded, fix issue #196 for this
  }


Looking at this function, if this.currentView.storageName is null or undefined, then the copyReport method is called with an empty data object. It is better to add that to the if block.
Also, the declaration and initialisation can be combined like so:

if (this.currentView.storageName) { const data: Record<string, number[]> = { [this.currentView.storageName]: [storageId] }; this.httpService.copyReport(data, 'Test').subscribe(); // TODO: storage is hardcoded, fix issue #196 for this }

MatthijsSmets · 2024-05-24T12:33:37Z

src/app/debug/table/table.component.ts

-          catchError(this.httpService.handleError());
-        },
-      });
+    if (this.viewSettings.currentView?.metadataNames && this.viewSettings.currentView?.storageName) {


Suggested change

if (this.viewSettings.currentView?.metadataNames && this.viewSettings.currentView?.storageName) {

if (

this.viewSettings.currentView &&

this.viewSettings.currentView.metadataNames &&

this.viewSettings.currentView.storageName

) {

MatthijsSmets · 2024-05-24T14:21:49Z

src/app/shared/interfaces/metadata.ts

+  endpointName: string;
+  timestamp: string;
+
+  // [key: string]: string | boolean;


please remove commented code

MatthijsSmets · 2024-05-24T14:23:23Z

src/app/shared/interfaces/target-with-files.ts

@@ -0,0 +1,3 @@
+export interface TargetWithFiles extends EventTarget {


Look at my comment about using HtmlInputElement, that way this interface is redundant and can be removed

MatthijsSmets · 2024-05-24T14:24:33Z

src/app/shared/interfaces/view-settings.ts

+
+export interface ViewSettings {
+  currentView?: View;
+  currentViewName?: string;


currentView already has an attribute called name

MatthijsSmets · 2024-05-24T14:26:04Z

src/app/shared/pipes/table-cell-type-checker.pipe.ts

+  name: 'tableCellTypeChecker',
+  standalone: true,
+})
+export class tableCellTypeChecker implements PipeTransform {


Suggested change

export class tableCellTypeChecker implements PipeTransform {

export class TableCellTypeChecker implements PipeTransform {

MatthijsSmets · 2024-05-24T14:29:16Z

src/app/test/test-folder-tree/test-folder-tree.component.ts

@@ -20,7 +20,7 @@ export class TestFolderTreeComponent {
    folderBehaviourOnClick: 'select',
    expandAllFolders: true,
    highlightOpenFolders: false,
-    autoSelectCondition: (item: CreateTreeItem) => this.autoSelectItem(item),
+    autoOpenCondition: (item: CreateTreeItem) => this.autoSelectItem(item),


autoOpenCondition is an entirely different function in the tree. Why did you change this?

dylan van dijk and others added 27 commits March 28, 2024 10:02

Assigned type Object[] to getMetadataReports()

dbf2564

added type "number" to count in loadMetadataCount()

9b4dc34

added typing to observable and get() of getMetadataCount function. Al…

1ba8ce1

…so added TODO comments for tracking progress

Added typing to Observable and get() of getUserHelp

ee7bf8f

added types to several functions

501f439

Added types to get- and resetSettings(). Interface "options-settings"…

9ad1681

… added to list of interfaces.

Added types to a lot of functions, mainly in http.service.ts

24d9783

All incoming data from the backend is now typed

ea4cbe2

Merge branch 'master' into origin/refactor/correct-type

58b22c9

Fixed linter/prettier issue

7b14465

Merge branch 'origin/refactor/correct-type' of https://github.com/wea…

72dc760

…refrank/ladybug-frontend into origin/refactor/correct-type

Changed Object types to be more descriptive

e0046b3

Fixed linter issues

1621221

Update src/app/shared/interfaces/options-settings.ts

01febef

Co-authored-by: Matthijs Smets <93487259+MatthijsSmets@users.noreply.github.com>

Update src/app/debug/table/table.component.ts

233ca9b

Co-authored-by: Matthijs Smets <93487259+MatthijsSmets@users.noreply.github.com>

Added return types to some functions and changed data from let to const

1258f14

fixed linter issues

911f785

fixed linter issues

78c1b8e

added types to functions utilizing getViews and getMetaDataReports

679c6b7

added types to functions that utilize http.service

47a9356

added types to functoins within table.component.ts and revised/added …

06eba06

…some interfaces

Added types to functions that utilize http.service, also added some i…

484ebc0

…nterfaces to help accomplish that.

Added types to functions that utilize http.service functions and adde…

03048c7

…d interfaces to help accomplish that.

Added interfaces and types to functions that

34d2747

utilize http.service functions.

all functions that utilize http.service functions are typed. as are a…

9299d24

…ll interfaces used to achieve type safety. only some issues left in table.component.html

all functions utilizing http.service are typed. trouble in table.comp…

0a574e3

…onent.html

Final commit

a1b6831

philipsens requested changes Apr 26, 2024

View reviewed changes

Merge branch 'master' into origin/refactor/retype-functions

240b7aa

Banshaan requested a review from philipsens April 26, 2024 12:02

Matthbo requested changes Apr 26, 2024

View reviewed changes

philipsens requested changes Apr 26, 2024

View reviewed changes

dylan van dijk added 2 commits April 30, 2024 09:45

Processed github suggestions into the code

10b42ea

Turned unnecessary 'let's into 'const's

5b49c3f

MatthijsSmets requested changes Apr 30, 2024

View reviewed changes

philipsens reviewed May 1, 2024

View reviewed changes

src/app/debug/table/table.component.ts Outdated Show resolved Hide resolved

dylan van dijk and others added 5 commits May 15, 2024 10:36

Incorporation of github comments

6a4cbe7

Working on fixing merge conflicts

eff2b6d

Working on fixing merge issues

fb521bd

Attempt at getting rid of merge conflicts

e72daf3

Merge branch 'master' into origin/refactor/retype-functions

6d73d71

dylan van dijk added 2 commits May 23, 2024 10:44

Hopefully last changes

116d781

Forgot to get rid of comment

1b2a12b

Matthbo previously approved these changes May 24, 2024

View reviewed changes

src/app/debug/display/display.component.ts Outdated Show resolved Hide resolved

Banshaan dismissed Matthbo’s stale review via c85d56c May 24, 2024 12:25

Update src/app/debug/display/display.component.ts

c85d56c

Added suggestion to turn a string concatenation into a template string Co-authored-by: Vivy <4380412+Matthbo@users.noreply.github.com>

Banshaan requested review from Matthbo, MatthijsSmets and philipsens May 24, 2024 12:31

philipsens reviewed May 24, 2024

View reviewed changes

Matthbo approved these changes May 24, 2024

View reviewed changes

Merge branch 'master' into origin/refactor/retype-functions

61160da

MatthijsSmets requested changes May 24, 2024

View reviewed changes

MatthijsSmets closed this May 27, 2024

	@Input() currentView: View = {};
	@Input() currentView!: View;

	this.helperService.download('id=' + queryString + '&', this.currentView.storageName, exportBinary, exportXML);
	this.helperService.download(`id=${queryString}&`, this.currentView.storageName, exportBinary, exportXML);

		@@ -0,0 +1,3 @@
		export interface TargetWithFiles extends EventTarget {

	export class tableCellTypeChecker implements PipeTransform {
	export class TableCellTypeChecker implements PipeTransform {

Origin/refactor/retype functions READY FOR REVIEW #391

Origin/refactor/retype functions READY FOR REVIEW #391

Conversation

Banshaan commented Apr 25, 2024

philipsens left a comment

Choose a reason for hiding this comment

Matthbo left a comment

Choose a reason for hiding this comment

MatthijsSmets 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

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

philipsens commented May 21, 2024

Matthbo left a comment

Choose a reason for hiding this comment

philipsens commented May 24, 2024

Choose a reason for hiding this comment

MatthijsSmets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthijsSmets May 24, 2024 • edited

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

MatthijsSmets May 24, 2024 •

edited