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

Add vertex class for javascript and typescript #370

Merged
merged 6 commits into from
Feb 21, 2023
Merged

Add vertex class for javascript and typescript #370

merged 6 commits into from
Feb 21, 2023

Conversation

zhuoqinyue
Copy link
Contributor

@zhuoqinyue zhuoqinyue commented Feb 15, 2023

If this PR is related to coding or code translation, please read the contribution guideline, fill out the checklist, and paste the console outputs to the PR.

  • I've tested the code and ensured the outputs are the same as the outputs of reference codes.
  • I've checked the codes (formatting, comments, indentation, file header, etc) carefully.
  • The code does not rely on a particular environment or IDE and can be executed on a standard system (Win, macOS, Ubuntu).

@vercel
Copy link

vercel bot commented Feb 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
hello-algo ⬜️ Ignored (Inspect) Feb 19, 2023 at 5:10AM (UTC)

@krahets
Copy link
Owner

krahets commented Feb 15, 2023

Hi @zhuoqinyue ! The Vertex class is used for the adjacency list implementation. Could you update the adjacency_list.js and ...ts referring to the Java code. Thanks!

@zhuoqinyue
Copy link
Contributor Author

Hi @zhuoqinyue ! The Vertex class is used for the adjacency list implementation. Could you update the adjacency_list.js and ...ts referring to the Java code. Thanks!

OK. But there will cause a problem when I write the code of graph_bfs.
The module.exports of js will cause other code in adjacency_list.js to execute. (B
ecause I need to import adjacency_list to finish graph_bfs)
Then the execution result of adjacency_list.js will be output first, and then graph_bfs will be executed.

Do you think this problem has a big impact?

@krahets
Copy link
Owner

krahets commented Feb 16, 2023

Yes. We'd better solve this. @justin-tse How do you think about it?

@zhuoqinyue
Copy link
Contributor Author

I have a opinion that we don't have to update the adjacency_list.js.
And just write a new adjacency_list class in the javascript/include folder.

@krahets
Copy link
Owner

krahets commented Feb 16, 2023

Yes. But it is not elegant because we will make a twin class.

Is there an entry point check in JS and TS? Like if __name__ == "__main__": in Python.

  • When running this file, the test case will be executed.
  • When importing this file as a module, the test case will NOT be executed.

@zhuoqinyue
Copy link
Contributor Author

It seems there no such method.

@zhuoqinyue
Copy link
Contributor Author

zhuoqinyue commented Feb 16, 2023

emm... One more solution.
Create a main function to wrap these test cases, and then don't execute the main function.
Let the user execute it by himself @krahets

@krahets
Copy link
Owner

krahets commented Feb 16, 2023

Better but not enough elegant haha!

@zhuoqinyue
Copy link
Contributor Author

zhuoqinyue commented Feb 16, 2023

I've updated the adjacency_list @krahets

@justin-tse
Copy link
Contributor

Yes. But it is not elegant because we will make a twin class.

Is there an entry point check in JS and TS? Like if __name__ == "__main__": in Python.

  • When running this file, the test case will be executed.
  • When importing this file as a module, the test case will NOT be executed.

@zhuoqinyue Thank you for the clear codes. @krahets I think we can use require.main to solve this problem?
Node.js, require.main === module
I have tested it, you could try it.@zhuoqinyue
JS:
image
TS is the same.

@justin-tse
Copy link
Contributor

@zhuoqinyue, I am using ts-node to test my .ts file and it seems to be working well. However, I'm curious to know what kind of problems you are experiencing.

@zhuoqinyue
Copy link
Contributor Author

zhuoqinyue commented Feb 18, 2023

It works a lot in JS, but doesn't work in TS.
ts-node throw a TSError
image

Your ts-node install in the global?

@zhuoqinyue
Copy link
Contributor Author

The result tell me that ts-node doesn't have require @justin-tse

@justin-tse
Copy link
Contributor

Yes, I think you should install @types/node, and try it.

@zhuoqinyue
Copy link
Contributor Author

npm i @types/node --global
image

ts-node throw the same TSError again.

@zhuoqinyue
Copy link
Contributor Author

zhuoqinyue commented Feb 18, 2023

But use npm i --save-dev @types/node, it works successfully.
image

image

@justin-tse
Copy link
Contributor

It works a lot in JS, but doesn't work in TS. ts-node throw a TSError image

Your ts-node install in the global?

@zhuoqinyue Good job! I think you misunderstand my meaning that I install ts-node in global but install @types/node in the devDependencies because it is only used in our local project. Your warning message tells you what to do.

@zhuoqinyue
Copy link
Contributor Author

Could you tell me your config about @types/node, ts-node and typescript, such as json, path and version.
Because I failed again by using npm i --save-dev @types/node . @justin-tse

@justin-tse
Copy link
Contributor

justin-tse commented Feb 18, 2023

But use npm i --save-dev @types/node, it works successfully. image

image

I see your last message which is a success. What's the problem now? The warning message?

The following is mine:

❯ tsc -v       
Version 4.8.4
❯ ts-node -v
v10.9.1
❯ npm list
hello-algo@ /Users/xf/src/justin-tse/open-source/hello-algo
└── @types/node@18.11.14 -> ./node_modules/_@types_node@18.11.14@@types/node

@zhuoqinyue
Copy link
Contributor Author

Fine. I found my fault.
I should npm init -y in folder hello-algo firstly, and then npm i --save-dev @types/node.
Maybe we should add some gitignore rules for node_modules package.json?

@justin-tse
Copy link
Contributor

OK, I think we don't push node_modules and package.json to this repository. These files are a little related to this project but are too big. You could give a comment before the line

// need to add the package @types/node contains type definitions for Node.js, npm i --save-dev @types/node
if (require.main === module)

@zhuoqinyue
Copy link
Contributor Author

.gitignore make git to ignore some file or some folder change, as follows:

  • before add rules node_modules/:
    image
  • after add rules node_modules/:
    image

But git will detect the change of. gitgnore.

@zhuoqinyue
Copy link
Contributor Author

OK, I think we don't push node_modules and package.json to this repository. These files are a little related to this project but are too big. You could give a comment before the line

// need to add the package @types/node contains type definitions for Node.js, npm i --save-dev @types/node
if (require.main === module)

I've updated it. @justin-tse

Copy link
Contributor

@justin-tse justin-tse left a comment

Choose a reason for hiding this comment

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

Thank you!@zhuoqinyue Clear codes! My suggestions to use explicit types or implicit types are:
It's generally a good practice to use explicit types for function parameters and return types, as well as for variables that can be of more than one type. However, for variables that can only have one type and where the type is clear from the value, implicit typing is often sufficient.

codes/typescript/chapter_graph/graph_adjacency_list.ts Outdated Show resolved Hide resolved
codes/typescript/chapter_graph/graph_adjacency_list.ts Outdated Show resolved Hide resolved
codes/typescript/chapter_graph/graph_adjacency_list.ts Outdated Show resolved Hide resolved
@zhuoqinyue
Copy link
Contributor Author

OK, I've updated it

Copy link
Contributor

@justin-tse justin-tse left a comment

Choose a reason for hiding this comment

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

Thank you!@zhuoqinyue

@krahets krahets merged commit b89ea3e into krahets:main Feb 21, 2023
@krahets krahets added code Code-related polish Decorative detail or feature labels Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code-related polish Decorative detail or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants