Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Check missing API functionalities for new P2P package #32

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

matjazv
Copy link
Contributor

@matjazv matjazv commented Dec 16, 2022

What was the problem?

How was it solved?

  • New missing_api.go file was created which includes all missing functions for P2P package used by the rest of engine code.
  • Old p2p package was replaced in imports in whole engine with new p2p/v2 package.

How was it tested?

  • glisk and lengine successfully builds.
  • All unit tests passed.

Note

  • We can now more easily see what's missing from API point of view.
  • Integration will be done in issue Integrate P2P package into the engine #19.
  • If needed (for some testing), it's trivial to change that engine will again use old P2P package.
  • We can inspect missing_api.go file while developing a new P2P package to check if something is still not being implemented.

@matjazv matjazv self-assigned this Dec 16, 2022
Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Isn't it still too early to replace the packages in other places? I think it's safer to replace at the end

pkg/p2p/v2/missing_api.go Show resolved Hide resolved
pkg/p2p/v2/missing_api.go Outdated Show resolved Hide resolved
pkg/p2p/v2/missing_api.go Show resolved Hide resolved
@matjazv
Copy link
Contributor Author

matjazv commented Dec 19, 2022

Isn't it still too early to replace the packages in other places? I think it's safer to replace at the end

Yes, this is also a valid option to do it at the end.

If we still want to keep those changes for a developer reference what's still missing in a new P2P package, we could create a new dev branch and continue to work on it, instead of using main branch. In that case main will remain clean.

Idea of this PR is to have collected all missing functionalities of an old P2P package in one place (missing_api.go Inside this file can be some useful comments and we may add some more later if needed). With this a developer can easily see what's missing or don't need to be implemented anymore. We've now also got an idea how many functions/structs are actually exposed from P2P package to the rest of the engine.

One option is also to ignore this PR and not merge it, so just leave it open for the reference as stated above.

I'm open to any of these options.

@shuse2
Copy link
Collaborator

shuse2 commented Dec 19, 2022

I think it's good to have the reference file as you created. if we created the feature branch, probably we couldve just replace it now, but i think the last integration will need a bit more than simple replacement.

let's keep the missing api file, but revert the change in other packages?

@matjazv
Copy link
Contributor Author

matjazv commented Dec 19, 2022

I think it's good to have the reference file as you created. if we created the feature branch, probably we couldve just replace it now, but i think the last integration will need a bit more than simple replacement.

let's keep the missing api file, but revert the change in other packages?

Very good idea! I'll revert all changes in engine part of the code and just leave newly created file (missing_api.go) in P2P package.

Yes indeed, when integration will happen it will surely be more than just replacing some function calls, etc.

@shuse2 shuse2 merged commit 1370f73 into main Dec 20, 2022
@shuse2 shuse2 deleted the 29-missing-API-functionalities branch December 20, 2022 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check missing API functionality for new P2P package
3 participants