Skip to content

Add release for suspected commits#200

Merged
GeekaN2 merged 34 commits into
masterfrom
suspected-commits
Jun 7, 2021
Merged

Add release for suspected commits#200
GeekaN2 merged 34 commits into
masterfrom
suspected-commits

Conversation

@GeekaN2

@GeekaN2 GeekaN2 commented Apr 5, 2021

Copy link
Copy Markdown
Member

Handle the add-release task and save the release to the database

Comment thread workers/release/types/add-release.ts Outdated
/**
* Git commit data needed to create a release
*/
export interface CommitData {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

не надо это в hawk.types вынести?

Comment thread workers/release/README.md Outdated
## Parsing scheme

1. User wants to deploy project
2. REWRITE THIS DOC PLEASE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Comment thread workers/release/src/index.ts Outdated
}, {
$set: {
catcherType: payload.catcherType,
commits: commits,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

коммиты будут сохраняться строкой?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

там JSON.parse сверху, это массив

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

image

Comment thread workers/release/src/index.ts Outdated
const parsedCommits: CommitData[] = JSON.parse(commits);

await this.db.getConnection()
.collection(this.dbCollectionName)

@nikmel2803 nikmel2803 May 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

чтобы TS знал что храниться в коллекции надо указать collection<Type>(name)
тогда не будет ошибок из-за отсутствия типизации

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

у тебя везде без этого толком типизация не работает

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this.db.getConnection()
         .collection(this.dbCollectionName)

вынести это в отдельное св-во, раз ты часто к этому обращаешься

@lgtm-com

lgtm-com Bot commented May 26, 2021

Copy link
Copy Markdown

This pull request introduces 2 alerts when merging 05ed16b into aaba31a - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

Comment thread workers/release/README.md
Comment thread workers/release/tests/index.test.ts Outdated
@@ -0,0 +1,188 @@
/* tslint:disable:no-string-literal */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

у нас же нет tslint 🤔

Comment thread workers/release/src/index.ts
const map = await mockBundle.getSourceMap();
const workerInstance = new ReleaseWorker();

const extendedInfo: SourceMapDataExtended[] = workerInstance['extendReleaseInfo']([ {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

лучше проверять всё на публичных методах. И проверять работу внутренних компонентах по итоговому результату. В данном случае, лучше закинуть файл с нужным названием, вызвать handle и посмотреть, успешно ли он его обработал

Comment thread workers/release/src/index.ts Outdated
const parsedCommits: CommitData[] = JSON.parse(commits);

await this.db.getConnection()
.collection(this.dbCollectionName)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

у тебя везде без этого толком типизация не работает

Comment thread workers/release/src/index.ts Outdated
const parsedCommits: CommitData[] = JSON.parse(commits);

await this.db.getConnection()
.collection(this.dbCollectionName)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this.db.getConnection()
         .collection(this.dbCollectionName)

вынести это в отдельное св-во, раз ты часто к этому обращаешься

Comment thread workers/release/src/index.ts Outdated
* @param releaseData - info with source map
*/
private async save(releaseData: SourceMapsRecord): Promise<ObjectId | null> {
private async saveSourceMapJS(releaseData: SourceMapsRecord): Promise<ObjectId | null> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. логика кажется слишком замудрёной. Приходится достаточно много времени тратить на то, чтобы во всём разобраться (я так этого и не сделал). Этот код требует рефакторинга. Может можно как-то оптимизировать логику, разнести на более понятные методы или типо того?
  2. Смущает нейминг saveSourceMap и saveSourceMapJS нейминг похожий, но ф-ии разные.

@lgtm-com

lgtm-com Bot commented Jun 1, 2021

Copy link
Copy Markdown

This pull request introduces 2 alerts when merging 8f1e559 into aaba31a - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

Comment thread workers/release/README.md Outdated
## Parsing scheme

1. User wants to deploy project
2. REWRITE THIS DOC PLEASE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Outdated

Comment thread workers/release/README.md Outdated
Comment thread workers/release/README.md Outdated
GeekaN2 and others added 2 commits June 6, 2021 14:13
Co-authored-by: Peter Savchenko <specc.dev@gmail.com>
Co-authored-by: Peter Savchenko <specc.dev@gmail.com>
Comment thread workers/release/src/index.ts Outdated
import { SourceMapDataExtended, SourceMapFileChunk, CommitData, SourcemapCollectedData, ReleaseDBScheme } from 'hawk.types';
/**
* Java Script source maps worker
* Java Script releases worker

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

только JS? он не абстрактный?

Comment thread workers/release/src/index.ts Outdated
Comment thread workers/release/README.md Outdated

1. User wants to deploy project
2. He runs deploy script on the server and it runs static builder, for example Webpack.
3. After Webpack finished his job, our **Webpack Plugin** gets a source maps for new bundles and sends them to us.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ссылку лучше вставить, мне кажется

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

на плагин

GeekaN2 and others added 3 commits June 7, 2021 12:39
@GeekaN2 GeekaN2 merged commit 9308d7d into master Jun 7, 2021
@GeekaN2 GeekaN2 deleted the suspected-commits branch June 7, 2021 12:54
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.

3 participants