Skip to content

[**Part-1**] neo-module/master#3232

Merged
shargon merged 30 commits intoneo-project:masterfrom
cschuchardt88:neo-module/master
May 21, 2024
Merged

[**Part-1**] neo-module/master#3232
shargon merged 30 commits intoneo-project:masterfrom
cschuchardt88:neo-module/master

Conversation

@cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented May 14, 2024 •
edited
Loading

Change Log

  • Moved neo-modules/src/ to directory src
  • Moved neo-modules/tests/ to directory tests
  • Configured .github workflows for tests.
  • Run dotnet format
  • Added neo-modules projects to neo.sln file.

Closes #3230

image

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

image

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@shargon @Jim8y @vncoelho @superboyiii

@cschuchardt88 cschuchardt88 marked this pull request as draft May 14, 2024 03:45
@cschuchardt88 cschuchardt88 marked this pull request as ready for review May 14, 2024 04:12
@shargon
Copy link
Member

shargon commented May 14, 2024

what is part 2?

shargon
shargon reviewed May 14, 2024
View reviewed changes
@shargon
Apply suggestions from code review
e13b8a2 
Not related to the PR
shargon
shargon reviewed May 14, 2024
View reviewed changes
shargon
shargon reviewed May 14, 2024
View reviewed changes
vncoelho
vncoelho reviewed May 14, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I see that you moved modules inside the root folder.
Why not src?

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented May 14, 2024

I see that you moved modules inside the root folder. Why not src?

Because I figured it would make more sense to keep them separate.

  1. Its unrelated to core development. (their plugins)
  2. Easier to find and maintain.

what is part 2?

Any of the commits above you don't want, would be part 2 or unrelated un-needed changes/fixes. Part 1 is just to make a working solution.

@vncoelho
Copy link
Member

vncoelho commented May 14, 2024

@cschuchardt88,
I think it should be in the SRC in this first moment, like the others.

Furthermore, avoid any other change in files as we had discussed previously.
Thus, use any change in editorconfig in an additional PR.

vncoelho
vncoelho reviewed May 14, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I think that using the name plugins instead of modules is also great for the folder

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented May 14, 2024 •
edited
Loading

At least with plugins name we limit our self to only allowing plugins in this folder. But that can be done In the sln file to show. It can be big directory with +13 items now being added. Becoming to hard to manage with unrelated items in src directory.

cschuchardt88
cschuchardt88 commented May 20, 2024
View reviewed changes
vncoelho
vncoelho reviewed May 20, 2024
View reviewed changes
cschuchardt88
cschuchardt88 commented May 20, 2024
View reviewed changes
@vncoelho
Copy link
Member

vncoelho commented May 20, 2024 •
edited
Loading

I think it will be correct with the last commit, just waiting for the tests

@vncoelho
Copy link
Member

vncoelho commented May 20, 2024

In principle, checking the logs, it looks like everything was fine.
However, the coverage summary is not directly appearing on the PR.

image

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented May 20, 2024 •
edited
Loading

In principle, checking the logs, it looks like everything was fine. However, the coverage summary is not directly appearing on the PR.

I believe if it doesn't than coverage didn't change.

@vncoelho
Copy link
Member

vncoelho commented May 20, 2024

For Plugins the coverage is correct.
"coverage/coveralls (push) - Coverage remained the same at 48.583%" -> This is from the last commit on neo-modules

@vncoelho
Copy link
Member

vncoelho commented May 20, 2024

The other also increased a little. I think it is all correct.
We just have some display problems here in the PR. We can fix later if they persist.

vncoelho
vncoelho reviewed May 20, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I am adjusting my tooling and will test in some minutes.

vncoelho
vncoelho reviewed May 20, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

#36 71.70 /usr/share/dotnet/sdk/8.0.203/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ConflictResolution.targets(112,5): error NETSDK1152: Found multiple publish output files with the same relative path: /opt/neoLib/src/Plugins/RpcServer/config.json, /opt/neoLib/src/Plugins/StateService/config.json. [/opt/neoLib/src/Plugins/StateService/StateService.csproj::TargetFramework=net8.0]

@cschuchardt88 cschuchardt88 changed the title [**MOVE**] neo-module/master [**Part-1**] neo-module/master May 20, 2024
@vncoelho
Copy link
Member

vncoelho commented May 20, 2024

For me it is ready to merge.

Code is the same as neo-modules apart from necessary changes.
Compilation, tests and publish are all good in my local environment.

However, code is not running due to recent PRs that introduces some problems on wallet.
I think we can merge and then fix in order to move forward.

@Jim8y
Copy link
Contributor

Jim8y commented May 20, 2024

If code is good, we can go with this pr already. Not necessarily to complete everything in one pr. But maybe we need a development branch? @shargon

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented May 20, 2024

I have #3244 for changes

@vncoelho
Copy link
Member

vncoelho commented May 20, 2024 •
edited
Loading

If code is good, we can go with this pr already. Not necessarily to complete everything in one pr. But maybe we need a development branch? @shargon

If we go to a development branch please revert the last 3 commits as well and migrate them to such branch.

@shargon
Copy link
Member

shargon commented May 20, 2024

Give me a couple of hours for the comparison

@vncoelho
Copy link
Member

vncoelho commented May 20, 2024

@Jim8y , I think we can try with the master branch a little bit more now that we have "all necessary components" here.
I think that by pushing a UT that tests node's ability to start a consensus and do basic operations will be very good. Maybe we can keep working on main branch in such case.

But I am not against using a dev branch.

roman-khimov
roman-khimov reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Code-wise (at 68e4146) I've noticed some s/this// changes and two lines moved in src/Plugins/RpcClient/Models/RpcInvokeResult.cs compared to master of neo-modules. Minor, but can be fixed to have cleaner initial state here and stylistic changes in separate PRs.

Can't comment on csproj/workflows changes, likely OK.

vncoelho
vncoelho requested changes May 20, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Perhaps the aforementioned problem I mentioned on discord was not introduced on 08f2dfc56762bb43be33e873bc3659bad368d4d6, but here.

In this case, please do not merge until I finish my tests because the error here may take time to solve otherwise.

vncoelho
vncoelho approved these changes May 20, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

The error was, in fact, a typo on the new configuration file name on Part-2 PR.
Thus, it is approved now with tests running as expected.

shargon
shargon approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

The changes with neo-modules are understandable, to reduce warnings only "this" and a few things were removed.

@shargon shargon merged commit 0f04c7f into neo-project:master May 21, 2024
@shargon shargon deleted the neo-module/master branch May 21, 2024 08:01
@AnnaShaleva AnnaShaleva mentioned this pull request May 21, 2024
Closed
Jim8y added a commit to Jim8y/neo that referenced this pull request May 25, 2024
@Jim8y
Merge branch 'latest-plugins' of github.com:Jim8y/neo into latest-plu…
4a96d1e 
…gins

* 'latest-plugins' of github.com:Jim8y/neo: (21 commits)
  fix: custom plugins won't shown by command `plugins` (neo-project#3269)
  COVERALL: Improve maintenance and readbility of some variables (neo-project#3248)
  Update nuget (neo-project#3262)
  [**Part-2**] Neo module/master fixes (neo-project#3244)
  Fix `dotnet pack` error (neo-project#3266)
  Fix and Update devcontainer.json to use Dockerfile  (neo-project#3259)
  Add optimization to template (neo-project#3247)
  Optimize plugin's models (neo-project#3246)
  fix CancelTransaction !signers.Any() (neo-project#3263)
  COVERALL: fix broken by changing report from lcov to cobertura (neo-project#3252)
  fix TraverseIterator count (neo-project#3261)
  Native: include DeprecatedIn hardfork into usedHardforks (neo-project#3245)
  [**Part-1**] `neo-module/master` (neo-project#3232)
  Make `ApplicationEngine.LoadContext` protection level `public` (neo-project#3243)
  improve parse method in neo-cli (neo-project#3204)
  Fix neo-project#3239 (neo-project#3242)
  Neo.CLI: enable hardforks for NeoFS mainnet (neo-project#3240)
  v3.7.4 (neo-project#3237)
  fix hardfork issues (neo-project#3234)
  Update src/Neo.CLI/CLI/MainService.Plugins.cs
  ...

# Conflicts:
#	src/Neo.CLI/CLI/MainService.Plugins.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roman-khimov roman-khimov roman-khimov left review comments

@shargon shargon shargon approved these changes

@Jim8y Jim8y Jim8y approved these changes

@vncoelho vncoelho vncoelho approved these changes

Assignees

@cschuchardt88 cschuchardt88

Labels

Waiting for Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate Neo-Modules back to Neo core

5 participants

@cschuchardt88 @shargon @vncoelho @Jim8y @roman-khimov