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

Why bytes depends on collections? #1724

Closed
volodya-lombrozo opened this issue Jan 31, 2023 · 18 comments · Fixed by #2106
Closed

Why bytes depends on collections? #1724

volodya-lombrozo opened this issue Jan 31, 2023 · 18 comments · Fixed by #2106
Labels

Comments

@volodya-lombrozo
Copy link
Member

While implementing #1723, I encountered a strange issue: the bytes object depends on both collections.bytes-as-array and collections.list objects, which greatly hampers testing capabilities as tests use bytes from the local filesystem through OyFake. Here's why:

  1. OyFake loads bytes from the local system at this path: src/main/eo/org/eolang/bytes.eo
  2. The file src/main/eo/org/eolang/bytes.eo holds the collections.bytes-as-array and collections.list objects.
  3. But OyFake fails to load these objects from the filesystem.

I know how to fix this with OyRemote (which will significantly decrease overall testing time actually), but my main concern is why are the collections objects outside of the eo-runtime library? I strongly believe that eo-runtime should be self-contained and that all other objects should depend on it. I suggest moving the collections objects into eo-runtime.

@yegor256
Copy link
Member

I suggest moving the collections objects into eo-runtime.

@volodya-lombrozo maybe it's better to refactor bytes to get rid of the dependency?

@volodya-lombrozo
Copy link
Member Author

volodya-lombrozo commented Jan 31, 2023

@yegor256 But we have that dependencies in float.eo, int.eo, switch.eo too. Apparently, we need that dependency in all of that objects and removing it could lead to code duplication between collections and eo-runtime/collections

@volodya-lombrozo
Copy link
Member Author

@yegor256 This issue blocks #1723

@volodya-lombrozo
Copy link
Member Author

@yegor256 reminder

@yegor256
Copy link
Member

@volodya-lombrozo I have a strong feeling that we can remove as-hash methods from all our primitives, like bytes, string, etc. This as-hash method is used only by the objects in org.eolang.collections package. They need to turn objects into "hash". Let them implement this hashing mechanism.

@maxonfjvipon
Copy link
Member

maxonfjvipon commented Feb 15, 2023

@yegor256 there are two more places where collections are used is eo-runtime - float.minus and int.minus:

# Difference between $ and x
[x...] > minus
  mapped. > x-neg
    list
      x
    [element]
      neg. > @
        element
  ^.plus > @
    ...x-neg

and I'm about to add similar functionality to +inf and -inf. So if we want to get rid of that dependencies we either should implement float.minus, int.minus, +inf.minus, etc in java or make these objects accept only one argument

@yegor256
Copy link
Member

@maxonfjvipon or we can use recursion here, instead of mapped

@maxonfjvipon
Copy link
Member

@yegor256 @volodya-lombrozo we got rid of eo-collection dependencies in int, float and switch.

It seems the last step to independent eo-runtime is as-hash object

What are your thoughts about it?

@yegor256
Copy link
Member

yegor256 commented Feb 23, 2023

@maxonfjvipon I believe that adding as-hash to all objects in eo-runtime was a mistake. This functionality of turning bytes into hashes must be implemented in the eo-collections package. Something like this:

eq.
  as-hash.
    QQ.collections.hashed
      "Hello, world!"
  01-2E-F3-3A

Here, 01-2E-F3-3A is the hash of "Hello, world!".as-bytes.

@maxonfjvipon
Copy link
Member

maxonfjvipon commented Feb 23, 2023

@yegor256 then I suggest the next plan:

  1. Create a ticket in eo-collections to implement hashing functionality in the package
  2. Replace all eo-runtime as-hashes to new eo-collections as-hashes
  3. Remove as-hashes from all objects in eo-runtime

WDYT?

@yegor256
Copy link
Member

@maxonfjvipon I think it's a reasonable plan

@volodya-lombrozo
Copy link
Member Author

@yegor256 @maxonfjvipon Indeed, I've checked other objects and it seems that we use hashing only in collections.

@yegor256
Copy link
Member

@maxonfjvipon we may use hashing somewhere else too, in the future, but this feature may be present in a decorator of an object, not in every object. Its implementation in all objects are identical at the moment.

@maxonfjvipon
Copy link
Member

@yegor256 where will be the right place for such decorator? If as-hash is used only in eo-collections now then it's not a ultimate reason to place it in eo-collections. Maybe there is a better place for it?

@yegor256
Copy link
Member

@maxonfjvipon I would keep it in eo-collections for now. Hashing function is needed primarily for hash maps and hash sets. If someone wants to use it in the future somewhere else, they will need to use it from eo-collections.

@maxonfjvipon
Copy link
Member

@yegor256 Ok, let's place it there

@mximp
Copy link
Contributor

mximp commented May 10, 2023

@volodya-lombrozo @maxonfjvipon
Is this issue waiting for objectionary/eo-collections#156 ?

@mximp mximp added the question label May 10, 2023
@maxonfjvipon
Copy link
Member

@mximp You're right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants