Skip to content

Conversation

DudaGod
Copy link
Member

@DudaGod DudaGod commented May 4, 2020

What is done:

  • add not really strict ts config for gradual migration;
  • rewrite all files from .js to .ts (basically only imports/exports have changed). All types will be added in next PR;
  • change npm scheme publish from blacklist to whitelist. Now publish only builded source files;
  • add script to copy image fixtures and current d.ts file (will be deleted in next PR);
  • remove unused benchmark folder;
  • add eslint with TS support.

@DudaGod DudaGod requested review from CatWithApple and sipayRT May 6, 2020 22:24
package.json Outdated
"prepare": "npm run clean && npm run build",
"test": "npm run build && npm run test-unit && npm run lint",
"test-unit": "mocha build/test --recursive --require ./build/test/setup",
"lint": "eslint . --ext .js,.jsx,.ts,.tsx",
Copy link
Contributor

Choose a reason for hiding this comment

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

зачем jsx и tsx тут?
в проекте не будет же UI

Copy link
Contributor

Choose a reason for hiding this comment

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

давайте добавим prettifier. потом добавить будет больнее

src/index.ts Outdated
import _ from 'lodash';
import parseColor from 'parse-color';
import colorDiff from 'color-diff';
import * as png from './lib/png';
Copy link
Contributor

Choose a reason for hiding this comment

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

вроде нет смысла делать import * as для файлов этого же пакета

Copy link
Member Author

Choose a reason for hiding this comment

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

Не понял. Почему не имеет смысла?

Copy link
Contributor

Choose a reason for hiding this comment

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

мне не нравится такая запись import * as. поэтому я бы предпочёл её избегать в меру сил и возможностей.
мб я один такой :)

@@ -1,6 +1,7 @@
'use strict';
export = class DiffArea {
Copy link
Contributor

Choose a reason for hiding this comment

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

давай сразу оторвём все export = и export default

Copy link
Member Author

Choose a reason for hiding this comment

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

не понял зачем везде исправлять export default? Я наоборот всегда его юзаю если предполагается, что модуль будет экспортить только один класс/функцию/...

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1,6 +1,7 @@
'use strict';
export = class DiffArea {
Copy link
Member Author

Choose a reason for hiding this comment

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

не понял зачем везде исправлять export default? Я наоборот всегда его юзаю если предполагается, что модуль будет экспортить только один класс/функцию/...

@DudaGod
Copy link
Member Author

DudaGod commented May 10, 2020

/fixed
В последнем коммите много изменений от запуска prettier-а.

},
"homepage": "https://github.com/gemini-testing/looks-same"
"name": "looks-same",
"version": "7.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

версия даунгрейднулась

"chai-as-promised": "^7.1.1",
"copy": "^0.3.2",
"eslint": "^6.8.0",
"eslint-config-gemini-testing": "^2.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

eslint-config-gemini-testing можно удалить, если не пользуемся

* It is created specially for the needs of visual regression testing for gemini utility, but can be used for other purposes.
*/
export = looksSame;
// Type definitions for looks-same 5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

а почему это отдельный файл .d.ts, а не просто ts код?

@@ -0,0 +1,87 @@
"use strict";
Copy link
Contributor

Choose a reason for hiding this comment

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

use strict не нужен

@@ -21,8 +19,8 @@ const validateBoundingBoxCoords = ({boundingBox}) => {
}
};

exports.validateImages = (img1, img2) => {
[img1, img2].forEach((i) => {
export const validateImages = (img1, img2) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

предпочёл бы export function validateImage ...

@sipayRT
Copy link
Member

sipayRT commented Aug 15, 2024

ping

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.

5 participants