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

Consolidate method/function names #44

Closed
grobie opened this issue Apr 21, 2017 · 10 comments
Closed

Consolidate method/function names #44

grobie opened this issue Apr 21, 2017 · 10 comments

Comments

@grobie
Copy link
Member

grobie commented Apr 21, 2017

The current naming is not stringent nor idiomatic.

From @mdlayher

My personal rules of thumb for each case:

  • NewThing: top level constructor functions. Never methods.
  • ParseThing: parses some input data like []byte or io.Reader.
  • Thing: retrieval methods for some data. Doesn't make much sense as a name for top-level functions to me.

It might make sense to just use the procfs names wherever possible.

@mdlayher
Copy link
Contributor

I'd also like to suggest that we get rid of constructor functions like NewBuddyInfo in favor of methods like: FS.BuddyInfo, because:

  • it will shrink the API surface
  • it will reduce redundancy
  • it will enable much easier testing

@grobie
Copy link
Member Author

grobie commented Apr 21, 2017

@mdlayher You mean all package level shorthand methods? Even Self()?

@mdlayher
Copy link
Contributor

Good question. I suppose it depends how much some of them are used. While they may be convenient, I imagine that most applications will want to construct a FS instance anyway for testing that doesn't only work on Linux machines.

@pgier
Copy link
Collaborator

pgier commented May 7, 2019

Is this something that we still want to do? After the refactoring in #149, we still have a lot of method names that look like fs.NewSomething(). Using the BuddyInfo example, we have NewBuddyInfo() and fs.NewBuddyInfo(). We could change the fs method to fs.BuddyInfo() however the top level function, can't be changed to just BuddyInfo() because then it conflicts with the BuddyInfo struct. Maybe we can change top level functions to something like ReadBuddyInfo() instead?

@discordianfish
Copy link
Member

I think the idiomatic way would be to call it BuddyInfo() and rename the struct instead..

@discordianfish
Copy link
Member

See the linked PRs. I think fs.BuddyInfo() is the way to go and I tend to say we should call the package level functions that do the same should also be called the same but that's somewhat against prefixing "top level constructor functions" with New.. @mdlayher Would love your 2 cents on this.

@mdlayher
Copy link
Contributor

mdlayher commented May 14, 2019

I still agree with my previous comment: #44 (comment)

I'd like to see very few package level functions, and everything is only accessible via a method on the FS type.

@pgier
Copy link
Collaborator

pgier commented May 15, 2019

I agree that it seems better to remove the top level constructor functions and just always require fs to make the API more consistent. So once the current set of PRs are merged, I'll create another PR to:

  1. remove the top level NewThing() constructor functions
  2. change the FS methods from NewThing() to Thing()
  3. since FS would now be required in most/all cases, maybe add a new FS constructor such as NewDefaultFS() for when /proc and /sys are in the default locations.

@discordianfish
Copy link
Member

Don't have a opinion on that, so if you think that's better - let's do it.

pgier added a commit to pgier/procfs that referenced this issue May 21, 2019
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue prometheus#44

Signed-off-by: Paul Gier <pgier@redhat.com>
pgier added a commit to pgier/procfs that referenced this issue May 21, 2019
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue prometheus#44

Signed-off-by: Paul Gier <pgier@redhat.com>
pgier added a commit to pgier/procfs that referenced this issue May 29, 2019
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue prometheus#44

Also includes some minor readme and godoc updates.

Signed-off-by: Paul Gier <pgier@redhat.com>
pgier added a commit that referenced this issue May 30, 2019
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue #44

Also includes some minor readme and godoc updates.

Signed-off-by: Paul Gier <pgier@redhat.com>
@pgier
Copy link
Collaborator

pgier commented May 30, 2019

Fixed in PR #162

@pgier pgier closed this as completed May 30, 2019
remijouannet pushed a commit to remijouannet/procfs that referenced this issue Oct 20, 2022
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue prometheus#44

Also includes some minor readme and godoc updates.

Signed-off-by: Paul Gier <pgier@redhat.com>
bobrik pushed a commit to bobrik/procfs that referenced this issue Jan 14, 2023
The `ProcError` error type now gains a new `InternalError` variant which
is designed to cover cases when the procfs library encounters something
unexpected and would otherwise panic.  Every instance of `InternalError`
should be considered a bug that should be reported

Closes prometheus#44
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