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

Exporting useful gen_server types #2690

Closed
wants to merge 3 commits into from

Conversation

galdor
Copy link

@galdor galdor commented Jul 18, 2020

Following an email on the erlang-questions mailing list, I'm opening this PR to discuss the possibility of exporting these types.

I believe adding these will avoid duplicate and error prone copy/pasting when writing type specifications for functions which spawn gen_server processes, or refer to them.

It is based on gen:emgr_name/0; the definition has been copied, the same way
it is done for gen_server:server_ref/0 (which is identical to
gen:server_ref/0).
@KennethL KennethL added the team:VM Assigned to OTP team VM label Aug 17, 2020
@uabboli uabboli assigned dgud and RaimoNiskanen and unassigned uabboli Aug 19, 2020
@garazdawi garazdawi added team:PS Assigned to OTP team PS and removed team:VM Assigned to OTP team VM labels Aug 24, 2020
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Sep 1, 2020
@paulo-ferraz-oliveira
Copy link
Contributor

Also check potentially related #2375.

@essen
Copy link
Contributor

essen commented Dec 29, 2020

Why is this marked as stalled?

@galdor
Copy link
Author

galdor commented Apr 18, 2021

Has there been any progress in the decision process regarding exporting these types ? I was really hoping to get them for OTP 24, but I'm starting to believe it will not be the case. This PR was open nine months ago. The diff is less than 10 lines long.

Being able to write accurate function specifications is important both for correctness (with Dialyzer) and for code documentation (this is quite obvious in lots of Erlang modules where the lack of type specifications hinders readability). Lots of standard Erlang modules do not export types (starting with gen_server, but also with re for example), and this situation is really annoying. The alternative is to copy/paste these definitions in each project which is insane.

The supervisor module is already exporting similar types, there is no reason not to do the same for gen_server.

@IngelaAndin
Copy link
Contributor

The problem is not in the principal but in the legacy. Just exporting the existing types without afterthought might cause unforeseen problems. And alas these kind of decisions needs coordination and possible involvement of may different people and alas takes a lot of time.

@galdor
Copy link
Author

galdor commented Apr 27, 2021

I have a lot of respect for the focus of the OTP team on stability and robustness which where qualities that brought me to Erlang in the first place. I'm however concerned by what seems to be the almost complete impossibility to move forward on various issues without delays going from several months to multiple years (I still hope for improvements in Dializer, one can dream :)).

When I look at this very PR or at others from various authors, it feels to me that trying to contribute is a waste of time and that I should invest my energy in building alternatives to OTP and various components of the standard library, just because I would be actually able to update them.

Hopefully this will improve with time :)

@IngelaAndin
Copy link
Contributor

I am sorry you feel that way. I do not think you are wasting your time. A lot of these PR coming in actually helps put focus on the problem. It is not always the number of lines changed that makes it easy or not to accept a PR. However our ways of working is constantly reviewed to make things better for everyone, so @KennethL I suppose we need to think of if we can improve the Open Source experience in cases such as this.

@vkatsuba
Copy link
Contributor

vkatsuba commented Apr 27, 2021

Slow accepting of PRs forces developers to bypass direct and reliable solutions, my opinion may not be heard or may not be supported but active participation in reviews from the OTP team could only improve the quality and improve the community. Eg PR #4775 was provided few hours ago and fix real issue for few products at the same time, but nobody does not focus on this and there is a high probability that the PR #4775 will be also waiting year or more for accepting, what could kill the hope of seeing this fix soon eg in next release. It would be incredibly cool if there was a dedicated group of developers from the OTP team that conducts reviews of open PRs every week, you need to spend only one hour a week to collect feedback and another hour to discuss this with the main team and voila fix or improvement already in release. But this is more view from the outside and can be ignored. 🙃

@galdor
Copy link
Author

galdor commented Apr 28, 2021

@IngelaAndin thank you for acknowledging this. Feel free to contact me if I can help clarify things regarding this PR, I'll be happy to help.

@vkatsuba to be fair, you cannot except any open source project to react to a PR in 13 hours, unless you start paying for this kind of support.

@lhoguin
Copy link
Contributor

lhoguin commented Apr 28, 2021

It is only a few lines but the debate around this dates back many years, perhaps even a decade. You don't solve a debate this long by submitting a 5 lines PR.

Personally the PR looks fine, except I have no idea why the request_id is exported. (But I also don't know what it does.) I also wonder why server_name is defined but not used, it sounds like it should just alias the relevant type in gen at the very least, to avoid duplicating. There's also a supervisor:sup_name that has the exact same thing. There's probably a better thing to do than add another duplicate of this. But that work could be done separately.

Eg PR #4775 was provided few hours ago and fix real issue for few products at the same time, but nobody does not focus on this

The OTP team has deadlines like everyone. Currently the focus is on getting OTP-24 done. They also have customers to support. And it's a small team considering the size of the product. It's not adequate to expect blazing fast reviews considering all this.

you need to spend only one hour a week to collect feedback and another hour to discuss this with the main team and voila

They likely already spend more time than that on average.

@galdor
Copy link
Author

galdor commented Apr 28, 2021

Personally the PR looks fine, except I have no idea why the request_id is exported. (But I also don't know what it does.)

Request ids are used to identify call requests; they are used when a process needs to reply asynchronously using gen_server:reply/2. Processes which do so have to keep track of these request ids.

I also wonder why server_name is defined but not used, it sounds like it should just alias the relevant type in gen at the very least, to avoid duplicating. There's also a supervisor:sup_name that has the exact same thing. There's probably a better thing to do than add another duplicate of this. But that work could be done separately.

I agree; the idea what to introduce a minimal change to see how it goes, before going crazy with big changes which cause unexpected issues down the way.

@IngelaAndin
Copy link
Contributor

@galdor One of the issues with exporting type spec for some of the functions is that it might document some things more precisely than we want it to because it should be sort of opaque, but not necessarily completely opaque as we might want to be able to match it for equality. Once we document something it is really hard to change so that is why we are cautious. We will try to address these issues but alas the OTP-24.0 time frame is too tight. But that said it does not mean we have to wait for OTP-25.

@IngelaAndin
Copy link
Contributor

@vkatsuba We do follow up on PR weekly. The problem is that we maintain a fairly large codebase for the number of people involved and parts that are not so actively developed will always take more time to investigate and can not always receive top priority.

@KennethL KennethL added the types The issue is related to types label Nov 22, 2021
@IngelaAndin IngelaAndin removed the stalled waiting for input by the Erlang/OTP team label Jan 26, 2022
@IngelaAndin
Copy link
Contributor

Closed in favor of #5751

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS types The issue is related to types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet