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

Add support for array type #2

Closed
SuperSandro2000 opened this issue Oct 22, 2021 · 6 comments · Fixed by #9
Closed

Add support for array type #2

SuperSandro2000 opened this issue Oct 22, 2021 · 6 comments · Fixed by #9

Comments

@SuperSandro2000
Copy link

It would be nice if basic python array types would be supported.

REDUCE requires a Callable object: &types.GenericClass{Module:"array", Name:"array"}

@matteo-grella
Copy link
Member

@marco-nicola

@SuperSandro2000
Copy link
Author

I did some more research about this the last days come up with the following additional information:

The type is imported using array and can then be used as array.array.

A poor implementation could look like the following snippet but as the comment already says it is incomplete

type Array types.List

var _ types.Callable = &Array{}

func (*Array) Call(args ...interface{}) (interface{}, error) {
	// TODO: make fully functional
	// args[0] contains a type like B or H which is not taken into account
	return args[1].(*types.List), nil
}

@marco-nicola
Copy link
Member

Hi, thanks for opening this issue.

I also think it would be nice to support this type (surely as much as I'd like to support many other types from the standard library, which are still missing... well, one step at a time).

While I'm not much used to manipulating arrays, their affinity with lists looks quite reassuring, at least at a first glance.

I think that your suggestion already goes in the right direction. Let's try to build additional ideas and opinions on top of it.

We surely need a new type, being Array a good name (duh!). We can probably place its own code in a new file inside the package types. We might even create an additional array sub-package, but I prefer to keep it simple for now, postponing the creation of separate sub-packages to a future time when the amount of implemented types will be more significant.

Rather than making the new Array type identical to List, we can define it as a struct containing both the array's type (a humble byte for the type-code should be enough) and the actual data. For the latter, I'm not yet sure whether we should reuse or even embed the existing List; I would probably decide that depending on the actual unpickling process (that said, I hope we can reuse the same type, indeed).

So, the full task to do here would be: to support the unpickling of Python's array.array type, making it work with all known type codes and all pickle protocols, also providing a set of test cases for those combinations.

Contributions are very welcome, so I ask you, @SuperSandro2000: would you like, feel confident, and have time to carry on with this task? Or at least a part of it? I can provide help and guidance if necessary, and of course a final review. Please, be honest and feel no pressure!

Liebe Grüße!

sbinet added a commit to sbinet-xyz/gopickle that referenced this issue Nov 17, 2023
sbinet added a commit to sbinet-xyz/gopickle that referenced this issue Nov 17, 2023
sbinet added a commit to sbinet-xyz/gopickle that referenced this issue Nov 17, 2023
@sbinet
Copy link
Collaborator

sbinet commented Nov 20, 2023

I know it's a bit late in the game, but: does #9 work for you, @SuperSandro2000 ?

@matteo-grella
Copy link
Member

Hey @sbinet do you see any cons on merging #9 as it is?

@sbinet
Copy link
Collaborator

sbinet commented Nov 20, 2023

the only possible issue is the support for utf16 and utf32 arrays which, as I said in the PR, might not be completely robust.
but that could be fixed in a later PR, if people notice something weird.

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 a pull request may close this issue.

4 participants