Typescript Conversion #13

ardelato · 2021-09-16T18:12:40Z

Description

This pull address #4 .

Changes

Converts all *.js to *.ts.
Ensures mostly complete test coverage for main files now.

lcov-report.zip
Adds typescript configuration and package.json scripts to setup and run the application.

CR Notes

The only major logic changes are:

Initing Pull objects with factory methods instead of through a constructor
- 74f35d2
- 0965924
extracting the parsing logic and adding it to the db_pull instead to make use of referencing object's properties instead of using a temporary object and replacing all values in the pull object with it.
- 090e755
changes to how we determine if a pull is currently dev blocked or not
- 9105014
- a27e042
Refactoring of tests -> pull repeated variables as global variables & additional tests for coverage

Other than that, it may look like a lot of changes, but if you go commit by commit, it should show that that the basic logic is still there, we are now just adding type validation.

QA

Validate you can run the application and run the tests locally on your machine.

Requirements

Have node 14 installed (use nvm to help manage your node versions)
Have a mysql server running
- Make sure to have a database that has a qa_pulls and qa_metrics table.
  - You can use the schema.sql file in the db folder to create the tables.
Have a .env.dev & a .env.test file in the root directory (ping me for a copy)
Have a config.ts in your config directory (ping me for a copy)

Steps

Git clone this repo locally
Check out this branch
Run npm install
Then run npm run lint:types
Validate you were able to run the script without issues and typescript did not report any errors
Run 'npm test'
Validate jest ran all the tests in __test__/ (day.spec.ts , pull.spec.ts) and all tests were passing
Run npm start
Validate the the application was successful in starting up
Validate the qa_pulls and qa_metrics has been populated with all OPEN pulls
Validate the Current Pulls Today readout shows the same number listed in the QA column of Pulldasher (Note: there might be a pull that is labeled Cryogenic Storage so it does not show up in Pulldasher and it has not been excluding from the parsing logic yet)

Closes #4

add the connection configuration directly into knex.js

…ished Now directly calling the knex connection to the db models instead

package.json start will now compile ts and js files into out folder and then run index from there add 'out' directory to gitignore updated tsconfig to allow for implicit any types for the time being

Added a tsc build script in package.json to individually run build command

removed the .js extension on the logger file

…DayValues anyways

It will make it easier and standard for calling a date from the function then to constantly call 'new Date(**)' everywhere

…module

Changed config-template.js to ts

… files

bhuminson

CR 🗡️

deltuh-vee · 2021-09-22T23:01:00Z

QA 🎬
I was able to run the application and most of the tests passed
deploy_block ✊
…except for one test(day.spec.ts)

ardelato · 2021-09-22T23:02:10Z

So @deltuh-vee and I ran into a very weird bug when running the tests.

It seems by using let a test would fail, but if we used const instead the test would pass? This also only occurred on his machine and not mine?

Veryable/__tests__/day.spec.ts

Lines 71 to 74 in 8f39632

    
           let data = await db('qa_metrics').select() 
        
           expect(data.length).toBe(1) 
        
           expect(data[0]).toMatchObject(newDay)

Anyone have an idea on why using const would make the test pass on his machine?

ardelato · 2021-09-22T23:18:16Z

dev_block ☁️ going to change the test variables to consts

…st table

…est table

ardelato · 2021-09-22T23:26:53Z

un_dev_block 🌻 replaced lets with consts so it should work now on your machine @deltuh-vee

deltuh-vee · 2021-09-22T23:40:59Z

dev_block ✊
I reran npm test and it's still not passing.

andyg0808 · 2021-09-23T20:22:37Z

@ardelato Is it possible that it's not correctly connecting to the database?

ardelato · 2021-09-23T20:33:01Z

@ardelato Is it possible that it's not correctly connecting to the database?

@andyg0808 actually we just debugged, it is an issue with Jest trying to run tests in parallel and so by using --runInBand cli option we were able to get the tests passing https://jestjs.io/docs/cli#--runinband

This was an issue on @deltuh-vee 's computer because it is an older macbook pro

ardelato · 2021-09-23T20:34:13Z

un_dev_block ☁️

deltuh-vee · 2021-09-23T20:44:28Z

QA 🎬
Tests now pass with when run sequentially and the current pulls listed match pulldasher.

bhuminson

CR 🗡️

andyg0808

Partial review; feel free to use your judgment on the suggestions or to delay them to happen in some other PR.

I'll try and finish the review Monday or so.

tsconfig.json

scripts/utils.ts

db/db_pull.ts

types/global.d.ts

db/db_pull.ts

src/index.js

db/db_pull.ts

andyg0808

Rest of CR.

andyg0808 · 2021-09-27T23:40:31Z

db/db_pull.ts

+  let dev_block = ''
  signatures.tags.forEach(tag => {
    let regex = new RegExp(tag.regex + signatures.emoji, 'i')
    if (regex.test(comment)) {
-      tags['dev_block'] = tag.state
+      dev_block = tag.state.toString()
    }
  })
+  return dev_block


I think you might be able to take advantage of find to simplify this code:

Suggested change

let dev_block = ''

signatures.tags.forEach(tag => {

let regex = new RegExp(tag.regex + signatures.emoji, 'i')

if (regex.test(comment)) {

tags['dev_block'] = tag.state

dev_block = tag.state.toString()

}

})

return dev_block

const tag = signatures.tags.find(tag => (new RegExp(tag.regex + signatures.emoji, 'i')).test(comment));

return tag?.state.toString()

Note that this will now return the first element of signatures.tags that matches, rather than the last. If I'm correct in believing that signatures.tags comes from the configuration, it's probably a bug to be order-sensitive in the first place, so if this change causes problems from that standpoint, we might want to think about changing how we're using the config.

db/db_pull.ts

andyg0808 · 2021-09-28T00:47:24Z

CR 🐟 through 309e32f
but I'm very suspicious that #13 (comment) is an issue.
deploy_block 👍 on that.

ardelato · 2021-09-28T17:25:01Z

un_deploy_block 👍 addressed all suggestions

ardelato added 30 commits September 14, 2021 15:39

Installing typescript as dev dependency

31e9cb3

Creating tsconfig file

29d894c

Using tsconfig bases to extend tsconfig.json

472b77d

Extending the tsconfig compiler options

ea77286

Specifying compiler directory for typescript

20128d2

Adding package.json script to lint typescript code

a4397a2

Moving main files to a src folder

675a898

Updated import paths for moved files

f31bb64

Removing migration file since Prisma will end up replacing knex

78b05ab

Removed knexfile since we won't use knex for migrations in the future

9c27b0b

add the connection configuration directly into knex.js

Removing db_manager as it just validated if the connection was establ…

5b84cc4

…ished Now directly calling the knex connection to the db models instead

Moving schema file into db directory

245380b

Forgot to update the usage of db_manager in the tests

9373539

Starting to convert knex.js to .ts

5c96235

Updating references to Knex.js and updating configs

1991132

package.json start will now compile ts and js files into out folder and then run index from there add 'out' directory to gitignore updated tsconfig to allow for implicit any types for the time being

Building out files now and running the tests from the compiled directory

89de4bc

Added a tsc build script in package.json to individually run build command

Added type to knex config

3e9c333

Changed graphql_config.js to typescript

bc59889

Changed graphql.js to ts

2f34822

removed import extension for graphql_config and fixed typescript error

b3aff3e

Changed logger.js to ts

2644896

Fixed typescript errors for logger.ts

dcff2f3

Changed db_day.js to ts

a7a5be2

Added a DayMetric type to the class and declared class variables

363b2eb

removed the .js extension on the logger file

Removed granular getter functions since they are accessed through get…

ba005c1

…DayValues anyways

Removed message property since e is unknown

eaa65ec

Pulled get dates function to external file

41327db

It will make it easier and standard for calling a date from the function then to constantly call 'new Date(**)' everywhere

Modified the mock of getDates now that it is called from an external …

5cfb857

…module

Changed db_pull.js to ts

d13e4b9

Removed defaultData object and replaced it with a PullRequest interface

e46ef1e

ardelato added 2 commits September 16, 2021 10:44

ignore config.ts

c213eff

Changed config-template.js to ts

Now going run jest tests from typescript files instead of compiled js…

8f39632

… files

bhuminson approved these changes Sep 17, 2021

View reviewed changes

deltuh-vee self-assigned this Sep 22, 2021

deltuh-vee added the QAing Under inspection for quality assurance label Sep 22, 2021

ardelato added 2 commits September 22, 2021 16:23

Replaced 'let' with 'const' for Day tests and removing all rows in te…

c241de9

…st table

Replaced 'let' with 'const' for Pull tests and removing all rows in t…

112a75c

…est table

ardelato removed the QAing Under inspection for quality assurance label Sep 22, 2021

Run tests in sequential order instead of parallel

309e32f

bhuminson approved these changes Sep 24, 2021

View reviewed changes

andyg0808 reviewed Sep 24, 2021

View reviewed changes

This was referenced Sep 27, 2021

Rename global.d.ts to github_types.ts and export the interfaces #14

Closed

Prisma ORM Integration #15

Merged

andyg0808 reviewed Sep 28, 2021

View reviewed changes

ardelato merged commit 5267355 into main Sep 28, 2021

ardelato added a commit that referenced this pull request Sep 28, 2021

Refactoring based off @andy0808 's suggestions in PR #13

f178da8

ardelato added a commit that referenced this pull request Sep 28, 2021

Refactoring based off @andyg0808 's suggestions in PR #13

0131160

ardelato deleted the clean-typescript-conversion branch September 28, 2021 22:58

ardelato mentioned this pull request Nov 18, 2021

Feature: Dockerize Veryable #17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript Conversion #13

Typescript Conversion #13

ardelato commented Sep 16, 2021 •

edited

Loading

bhuminson left a comment

deltuh-vee commented Sep 22, 2021

ardelato commented Sep 22, 2021

ardelato commented Sep 22, 2021

ardelato commented Sep 22, 2021

deltuh-vee commented Sep 22, 2021

andyg0808 commented Sep 23, 2021

ardelato commented Sep 23, 2021 •

edited

Loading

ardelato commented Sep 23, 2021

deltuh-vee commented Sep 23, 2021

bhuminson left a comment

andyg0808 left a comment

andyg0808 left a comment

andyg0808 Sep 27, 2021

andyg0808 commented Sep 28, 2021

ardelato commented Sep 28, 2021

Typescript Conversion #13

Typescript Conversion #13

Conversation

ardelato commented Sep 16, 2021 • edited Loading

Description

Changes

CR Notes

QA

bhuminson left a comment

Choose a reason for hiding this comment

deltuh-vee commented Sep 22, 2021

ardelato commented Sep 22, 2021

ardelato commented Sep 22, 2021

ardelato commented Sep 22, 2021

deltuh-vee commented Sep 22, 2021

andyg0808 commented Sep 23, 2021

ardelato commented Sep 23, 2021 • edited Loading

ardelato commented Sep 23, 2021

deltuh-vee commented Sep 23, 2021

bhuminson left a comment

Choose a reason for hiding this comment

andyg0808 left a comment

Choose a reason for hiding this comment

andyg0808 left a comment

Choose a reason for hiding this comment

andyg0808 Sep 27, 2021

Choose a reason for hiding this comment

andyg0808 commented Sep 28, 2021

ardelato commented Sep 28, 2021

ardelato commented Sep 16, 2021 •

edited

Loading

ardelato commented Sep 23, 2021 •

edited

Loading