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

[Fix #116] Add types for some functions in process.c #117

Closed

Conversation

DamienCassou
Copy link
Contributor

No description provided.

@Fuco1
Copy link
Member

Fuco1 commented Oct 31, 2018

Thanks for this! But let's wait until #74 is solved so we don't have to re-do this again. I'm not sure if we can get a feasible automatic script going faster than doing the convertion manually.

I want to finish the proposal over the weekend and do the transition.

@DamienCassou
Copy link
Contributor Author

DamienCassou commented Nov 1, 2018 via email

@Fuco1
Copy link
Member

Fuco1 commented Nov 1, 2018

Actually you're right. I can modify the pretty-printer and then write some crazy regexp/reader while loop that would go over the source code.

I think I could do that in under an hour.

@Fuco1
Copy link
Member

Fuco1 commented Nov 1, 2018

Can you copy the process.c section from https://github.com/emacs-elsa/Elsa/blob/master/dev/builtin.txt#L649 and replace the lines you implemented with the code and keep the rest commented.

This is so we don't miss anything.

Then I think we can go ahead and merge as per the discussion above.

@DamienCassou
Copy link
Contributor Author

Can you copy the process.c section...

done.

Is it possible to define a type Process before merging that PR or within that PR? I'm a bit annoyed by this kind of signature:

(put 'get-process 'elsa-type (elsa-make-type String -> Mixed))

because Mixed should indeed be Process instead.

@Fuco1
Copy link
Member

Fuco1 commented Nov 1, 2018

There's an issue for that #31

I should really copy all the types there so we can track the progress easier, currently I only link to the categories.

To add a new primitive types this should be all that's required:

(defclass elsa-type-buffer (elsa-type) ())

(cl-defmethod elsa-type-describe ((this elsa-type-buffer))
  "Buffer")

That is, the class to represent the type and the describe method (pretty printer). The type constructor is then the suffix after elsa-type- capitalized.

Go ahead and add it to this PR, so long as it is a separate commit I have no problem with that.

@DamienCassou
Copy link
Contributor Author

I added all types in #118.

@DamienCassou DamienCassou force-pushed the add-process-builtins branch 2 times, most recently from ffe5c9c to 198d3c0 Compare November 3, 2018 11:22
@DamienCassou
Copy link
Contributor Author

I updated the PR with the new Process type. Some questions:

  • what should I specify as return type if the documentation doesn't say anything? For example, set-process-filter. Looking at the source code, the second parameter seems to be returned but I'm not sure we should use that as a type.
  • should I keep repeating (Process -> String -> Nil) as parameter or return value, or should I define a ProcessFilter type?
  • what should I do about optional arguments such as in (process-contact PROCESS &optional KEY)?
  • what should I do to specify a PList type of symbol keys and mixed values? E.g., in process-plist or set-process-plist.
  • what should I do to specify make-process which takes key/value pairs as an &rest ARGS.

@Fuco1
Copy link
Member

Fuco1 commented Nov 3, 2018

If we don't know the return type use Mixed for now. I'm still not sold on the void concept.

About the ProcessFilter, there is no way to declare custom types now, I've just realized the same thing and opened #119 literally 3 minutes ago :D And I would prefer not to have these defined with the defclass stuff. We should have a way to build types from the primitive types somehow.

Right now optional arguments are the same as nullable but it's not really the same thing. Declare it nullable and add a TODO that we need to fix it later.

For the last two, we don't have support for plists so go with lists of mixed for now.

I'll link this to the typespec issue since these are valid issues to be addressed.

@Fuco1 Fuco1 mentioned this pull request Nov 3, 2018
elsa-typed-builtin.el Outdated Show resolved Hide resolved
@DamienCassou
Copy link
Contributor Author

I did my best to cover all functions (except process-connection which is disabled in the C source code).

@Fuco1
Copy link
Member

Fuco1 commented Nov 3, 2018

@DamienCassou Sweet, one less to go. I'll review it but I don't expect much issues.

@Fuco1
Copy link
Member

Fuco1 commented Mar 2, 2019

I've rebased and merged the commits into master.

@Fuco1 Fuco1 closed this Mar 2, 2019
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.

2 participants