Skip to content

Conversation

JeneaVranceanu
Copy link
Collaborator

Updated module name from Core to Web3Core because in 3rd party projects it doesn't make sense to import the module named Core. It doesn't tell anything about what the module actually is.

For example, if you want to use only EthereumAddress in some file of your project you need to add import Core among the imports. Using import web3swift will result in Cannot find type 'EthereumAddress' in scope because Core is a different module.
Using both web3swift and Core still doesn't explain what Core is.

@JeneaVranceanu
Copy link
Collaborator Author

I'm considering also renaming Utilities into Web3Utilities or at least providing a typealias Web3Utilities = Utilities.
It is a name that could overlap with other libraries or developers' projects.
Example use cases:
Screenshot 2022-12-03 at 16 41 46
Screenshot 2022-12-03 at 16 41 53

@janndriessen
Copy link
Collaborator

Good suggestions. 👌 I would argue why have even web3swift and (web3)core separate? What was the intention behind it?

p.s. This will be a fun one to rebase. 😅

@JeneaVranceanu
Copy link
Collaborator Author

Good suggestions. 👌 I would argue why have even web3swift and (web3)core separate? What was the intention behind it?

p.s. This will be a fun one to rebase. 😅

The intention was to separate essential parts of the code from secondary (e.g. ERC standard implementations), mainly for convenience and clarity for those who contribute to web3swift repository.

@yaroslavyaroslav
Copy link
Collaborator

yaroslavyaroslav commented Dec 4, 2022

Good suggestions. 👌 I would argue why have even web3swift and (web3)core separate? What was the intention behind it?

Heh, this is actually because of you and only you)

It was around mid of August when I've raised question about whether it worth it to split lib into a modules or not? And there was just you who answered back there and your answer was yes)

Anyway I'm kidding of course, the intent of core one is to give ability to the developer folks use just very necessary minimum within their app if they wanted to rather then all big-fat library instead. So this would comes to stripping their binary size. And the actual Core state is the best I can get of that.

Also that refactoring helped to decoupling a lot of internal implementations. Though it's not complete from my point of view, I've wanted to split it even more granularly:

  • core
  • tokens
  • keystone
  • abi

But this one could lasts forever, so I ends up with this.

@janndriessen
Copy link
Collaborator

Heh, this is actually because of you and only you)

It was around mid of August when I've raised question about whether it worth it to split lib into a modules or not? And there was just you who answered back there and your answer was yes)

Anyway I'm kidding of course, ...

🤣👌

@yaroslavyaroslav
Copy link
Collaborator

@JeneaVranceanu while I see the issue, and agree that the module name was taken a bit excessively, and even more then that, having this issue #651 raised recently from other developer folks, I'd wondering, is it worth it to rename it now, while keeping in mind all those developer folks who already uses this lib and who has to refactor their code base, yep, I estimate them as a little, but they still are.

Also this issue stands straight mostly in Cocoapods, as well, as it would take some time to update it this way by me (means renamed core likely should be pushed there as new pod), because swift 5.7 provides module aliasing, I've not tested it by myself, but my guess is that it depends on toolchain version installed, so it should be available to all whom has latest Xcode installed.

On the other hand the issue is real, as I don't feel it, I see the folks that it hurts, and if we would made that change we should do it now or at 4.0.0.

So I'd like to hear a bit more about how bad this issue is before accepting it.

@JeneaVranceanu
Copy link
Collaborator Author

Note: by user I mean a developer who uses the library.

TL;DR The name is quite vague and unclear. People will complain and we can do better.

@yaroslavyaroslav
From the end-user perspective (users that have already bumped the web3swift version to 3..) the same changes will be required no matter if moduleAliasing is applied or if we just rename the module ourselves. It will result in a renaming of the Core module across their projects' source files.
And I think this is not the case where module aliasing should be applied. We have a module name that is too broad and unclear. It's not the issue of overlapping module names breaking the build.
And especially considering that there already was at least a question about the name in #651 - it will only get worse from here as people will raise questions and complain about the fact that they cannot use types like EthereumAddress because they are "not found" and someone will have to point out that import Core is missing.
We have to account for the dumbest scenarios and prevent things we can before minor issues spread like a disease.

I'm currently migrating a project to 3.. and it took me 20-30 seconds to realize that the module I need to import was Core. And that's considering I'm involved in the development of this library.
For newcomers, the careful reading of README.md will save them time but again I do not want to rely on that.

@JeneaVranceanu
Copy link
Collaborator Author

GitHub insights says that we have 258 unique git cloners. Lets treat that number as a number of active users though I'm sure it's lower than that. Publishing a new update we can add enough description to it clearly highlight that people should replace import Core with import WhatEverWeComeUpWith. And that's all they will need to do.
The web3swift module I'd like to rename to the Web3Swift but we can keep it web3swift. There is no confusion about what this module is related to.

Screenshot 2022-12-04 at 13 53 03

@yaroslavyaroslav
Copy link
Collaborator

@JeneaVranceanu sounds convincing, have nothing to say against it.

janndriessen
janndriessen previously approved these changes Dec 5, 2022
Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

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

✅ If we have multiple modules this def makes sense.

If I understood correctly now this was mainly done for contributors. I'd argue though that for users of the library, multiple modules are not really convenient. So this might be something to discuss in the future e.g. for v4.

# Conflicts:
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+Call.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+EstimateGas.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+FeeHistory.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetAccounts.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetBalance.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetBlockByHash.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetBlockByNumber.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetBlockNumber.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetCode.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetGasPrice.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetTransactionCount.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetTransactionDetails.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+GetTransactionReceipt.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+SendRawTransaction.swift
#	Sources/web3swift/EthereumAPICalls/Ethereum/Eth+SendTransaction.swift
#	Tests/web3swiftTests/remoteTests/ST20AndSecurityTokenTests.swift
Copy link
Collaborator

@yaroslavyaroslav yaroslavyaroslav left a comment

Choose a reason for hiding this comment

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

I'd approve everything, but the gitignore change, is it by purpose?

.gitignore Outdated
Comment on lines 75 to 76
# Sourcetree app
icon.png
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using Sourcetree and it allows droping an image for the project that Sourcetree picks up and displays in the UI.
It should be ignored as not everyone needs it and each developer could have it's own icon for the project.

Screenshot 2022-12-12 at 11 00 10

Copy link
Collaborator

@yaroslavyaroslav yaroslavyaroslav Dec 12, 2022

Choose a reason for hiding this comment

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

Maybe is it worth to put such into ~/.gitignore_global? I mean that this is your setup specific, not the tools that is used into project itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍
Removed from .gitignore.

@yaroslavyaroslav yaroslavyaroslav merged commit 8cffa32 into web3swift-team:develop Dec 14, 2022
@JeneaVranceanu JeneaVranceanu deleted the fix/module-name branch December 14, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants