Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOF-7096: Typescript #92

Merged
merged 22 commits into from
Dec 1, 2023
Merged

SOF-7096: Typescript #92

merged 22 commits into from
Dec 1, 2023

Conversation

k0stik
Copy link
Member

@k0stik k0stik commented Nov 7, 2023

No description provided.

package.json Outdated
"eslint-plugin-simple-import-sort": "^7.0.0",
"eslint-plugin-react": "7.30.0",
"eslint-plugin-simple-import-sort": "7.0.0",
"eslint-import-resolver-exports": "^1.0.0-beta.2",
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace these with the eslint plugin from npm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Am afraid we need all of this. From README

1. Install config with eslint and eslint plugins

npm install --save-dev \
  @exabyte-io/eslint-config \
  @babel/eslint-parser@7.16.3 \
  @babel/plugin-proposal-class-properties@7.16.0 \
  @babel/preset-env@7.16.4 \
  @babel/preset-react@7.16.7 \
  @babel/register@^7.16.0 \
  @babel/runtime-corejs3@7.16.8 \
  @typescript-eslint/eslint-plugin@5.9.1 \
  @typescript-eslint/parser@5.9.1 \
  eslint@7.32.0 \
  eslint-config-airbnb@19.0.2 \
  eslint-config-prettier@8.5.0 \
  eslint-import-resolver-exports@^1.0.0-beta.2 \
  eslint-import-resolver-meteor@^0.4.0 \
  eslint-import-resolver-node@^0.3.6 \
  eslint-plugin-import@2.25.3 \
  eslint-plugin-jsdoc@37.1.0 \
  eslint-plugin-jsx-a11y@6.5.1 \
  eslint-plugin-prettier@4.2.1 \
  eslint-plugin-react@7.30.0 \
  eslint-plugin-simple-import-sort@7.0.0 \
  eslint-plugin-mui-path-imports

import { LATTICE_TYPE_CONFIGS } from "./types";
import { LATTICE_TYPE_CONFIGS, LatticeType, Vector, VectorsAsArray } from "./types";

export interface BravaisConfig {
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to get this and the below from ESSE schema, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Replaced this with LatticeImplicitSchema

import { BravaisConfig } from "./lattice_bravais";
import { LatticeType, Vector } from "./types";

interface LatticeVectorsConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

@k0stik k0stik Nov 10, 2023

Choose a reason for hiding this comment

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

Replaced with LatticeExplicitUnit

TRI_2a: "TRI_2a",
TRI_1b: "TRI_1b",
};
export enum LatticeType {
Copy link
Member

Choose a reason for hiding this comment

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

Line 16 no longer relevant

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

RHL = "RHL",
MCL = "MCL",
MCLC = "MCLC",
TRI = "TRI",
Copy link
Member

Choose a reason for hiding this comment

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

Any way we could move these enums to ESSE also, to be able to potentially reuse in Python in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have defined LatticeType in ESSE LatticeImplicitSchema, so we can use them from there.
But I didn't find LatticeTypeExtended there. We can define another schema for this

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: we will be using strings instead of enums
bcherny/json-schema-to-typescript#200

src/types.ts Outdated
import { ConstrainedBasisJSON } from "./basis/constrained_basis";
import { LatticeJSON } from "./lattice/lattice";

export interface MaterialJSON extends AnyObject {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

(x) => math.unit(x, "degree").to("rad").value,
);

const [cosAlpha, cosBeta, cosGamma] = [alpha, beta, gamma].map((x) => math.cos(x));
const [sinAlpha, sinBeta, sinGamma] = [alpha, beta, gamma].map((x) => math.sin(x));
const [sinAlpha, sinBeta] = [alpha, beta, gamma].map((x) => math.sin(x));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove gamma from the right side as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/material.ts Outdated
return this.toJSONDefault();
interface MaterialSchemaJSON extends MaterialSchema, AnyObject {}

interface CheckResult {
Copy link
Member

Choose a reason for hiding this comment

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

We could take this from esse as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/material.ts Outdated
super(config);
this._json = lodash.cloneDeep(config || {});
}
interface Property {
Copy link
Member

Choose a reason for hiding this comment

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

Should be from ESSE too

Copy link
Member Author

@k0stik k0stik Nov 30, 2023

Choose a reason for hiding this comment

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

DerivedPropertiesSchema doesn't have name and value properties but we use them in getDerivedPropertyByName() and getInchiStringForHash().
Should I add it to to esse schema?

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved

@k0stik k0stik merged commit e90e345 into main Dec 1, 2023
4 checks passed
@k0stik k0stik deleted the update/SOF-7096 branch December 1, 2023 12:36
VsevolodX pushed a commit that referenced this pull request Jul 12, 2024
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