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

break the method convert #813

Closed
wants to merge 3 commits into from
Closed

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Nov 11, 2023

This pr changes no logic to the compiler but only break the 5000 line methodconvert file into multiple subfiles.

For the ease of maintaing the compiler and possibly add more features to the compiler, we need to break this super huge file to make it clear how neo compile C# into NEF.

Why we need this pr?

  1. we need to have a detailed document to tell the developers what part of C# is supported, having that document requires a thourough understanding of the compiler. And making the compile process clear helps us understand the code.

  2. Current compiler still misses some of C# features, we need a clear structure to check what is missing.

@Jim8y Jim8y marked this pull request as draft November 12, 2023 03:24
…to break-converter

* 'master' of github.com:neo-project/neo-devpack-dotnet:
  Set nullableContextOptions (neo-project#687)

# Conflicts:
#	src/Neo.Compiler.CSharp/MethodConvert.cs
@Jim8y Jim8y marked this pull request as ready for review November 12, 2023 03:28
@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 12, 2023

@shargon please review

@shargon
Copy link
Member

shargon commented Nov 12, 2023

We said first bugs then optimization a features xD

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 12, 2023

We said first bugs then optimization a features xD

@shargon this is exactly for adding tests, comments, documents, and checking bugs.

Take a look how easy it became to check what test is missing:

Screenshot 2023-11-12 at 8 15 48 PM

@Jim8y Jim8y requested a review from shargon November 12, 2023 12:54
@shargon
Copy link
Member

shargon commented Nov 12, 2023

@Liaojinghui neo-project/neo#2955 Point 2 🙄

Also, we already agree to focus on bugs first.

There are already tons of name style issues, comments issues, typo issues. Let's focus on fixing bugs please. neo-project/neo-modules#815 (comment)

Review this PR require to review all the 58 changed files, it spend a ton of time for ensuring that the logic was not changed, I'm agree that it's better like you did, but small PR are easy to review.

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 12, 2023

Not changing a single line of code, just split the big file. Not in a hurry.

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 15, 2023

I will close this pr to break the changes step by step into smaller prs.

@Jim8y Jim8y closed this Nov 15, 2023
@Jim8y Jim8y mentioned this pull request Nov 15, 2023
15 tasks
@Jim8y Jim8y deleted the break-converter branch March 3, 2024 02:39
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.

2 participants