mkellerman / PSTwitterAPI

PSTwitterAPI - a PowerShell module for the Twitter API
https://mkellerman.github.io/Introducing_PSTwitterAPI/
45 stars 13 forks source link

Remove all `_` from function names #29

Closed kilasuit closed 4 years ago

kilasuit commented 5 years ago

It is a standard practice that no _ or - (other than between Verb-Noun ) are used in function names

Therefore this issue is to raise about removing them to fall inline with standard PowerShell Practices

nohwnd commented 5 years ago

It is also a standard that the noun part is singular.

From the examples given in

Get-TwitterFavorites_List
Get-TwitterFollowers_Ids
Get-TwitterFollowers_List
Get-TwitterFriends_Ids
Get-TwitterFriends_List
Get-TwitterFriendships_Incoming
Get-TwitterFriendships_Lookup
Get-TwitterFriendships_NoRetweetsIds
Get-TwitterFriendships_Outgoing
Get-TwitterFriendships_Show

I think it would look better if the names were split on _ singularized, and the part after _ would become a switch (this opens up options how to implement this, one of them is distinct parameter sets).

mkellerman commented 5 years ago

I'm inviting everyone to voice their opinion on this.

I like the current function name structure. But I can easily change all the function names.

What do you think?

mkellerman commented 5 years ago

It is also a standard that the noun part is singular.

The issue with making them singular, is that they wouldn't match the API endpoint. https://developer.twitter.com/en/docs/accounts-and-users/follow-search-get-users/api-reference/get-friends-list.html

thomasrayner commented 5 years ago

Imo, it doesn't matter what the API endpoint is. The whole point of writing PS to wrap them is that you abstract their API away into something that's familiar to PS users.

I think @nohwnd is right in his suggestion to move to singular nouns and to have the post-underscore part moved into switches for one function rather than a bunch of functions.

mkellerman commented 5 years ago

I think it would look better if the names were split on singularized, and the part after would become a switch (this opens up options how to implement this, one of them is distinct parameter sets).

I like the idea, having less functions, and more dynamic parameters. but then, I might as well wrap everything into one function, and have a parameter for -endpoint 'friends/list' -<dynamicparams>

mkellerman commented 5 years ago

Imo, it doesn't matter what the API endpoint is. The whole point of writing PS to wrap them is that you abstract their API away into something that's familiar to PS users.

Not a big fan of making the module 'different' that the API. The idea is to facilitate the interaction with the API, not invent a whole new naming strategy for each endpoint.

ChrisLGardner commented 5 years ago

The idea is to abstract the api implementation away from the user so they don't need to know that the endpoint is called a specific thing. So having singular commands for each endpoint and then parameters to deal with list or ids is the way I'd handle it.

Under the hood they might all call your invoke-twitterapi command but the user doesn't need to see/use that.

mkellerman commented 5 years ago

I like the idea of exposing less functions. And having grouped functions with more dynamic parameters.

The functions are currently generated dynamically from the Twitter API Documentation, using the scripts in the APIHelper folder. If anyone wants to 'engineer' a solution to do it all dynamically, you're more than welcome to make a PR for it.

We could always keep the ~125 helper functions, put them in /private/ and add these dynamic helper functions in the /public/ section? Anyone else has any suggestions?

If having _ is making some of you guys die inside, I'll update the script to remove them. That's an easier fix for now.

kilasuit commented 5 years ago

@mkellerman it may be worthwhile for you to share this script that is dynamically creating these because as far as I was aware Twitter haven't implemented Swagger/OpenAPI documentation. (Actually I just seen the comment above about the API helper folder)

This also raises the concern that if Twitter change their API (which they have been know to do and break things) then if you are building form a json file where and how is this file generated?

kilasuit commented 5 years ago

Using what @nohwnd had in the example this

Get-TwitterFavorites_List
Get-TwitterFollowers_Ids
Get-TwitterFollowers_List
Get-TwitterFriends_Ids
Get-TwitterFriends_List
Get-TwitterFriendships_Incoming
Get-TwitterFriendships_Lookup
Get-TwitterFriendships_NoRetweetsIds
Get-TwitterFriendships_Outgoing
Get-TwitterFriendships_Show

should become


Get-TwitterFavorites
Get-TwitterFavorites -List

Get-TwitterFollowers
Get-TwitterFollowers -Id
Get-TwitterFollowers -List

Get-TwitterFriend
Get-TwitterFriend -Id
Get-TwitterFriend -List

Get-TwitterFriendship
Get-TwitterFriendship -Incoming
Get-TwitterFriendship -Lookup
Get-TwitterFriendship -NoRetweetsIds
Get-TwitterFriendship -Outgoing
mkellerman commented 5 years ago

@mkellerman it may be worthwhile for you to share this script that is dynamically creating these because as far as I was aware Twitter haven't implemented Swagger/OpenAPI documentation. (Actually I just seen the comment above about the API helper folder)

https://github.com/mkellerman/PSTwitterAPI/tree/master/APIHelper

This also raises the concern that if Twitter change their API (which they have been know to do and break things) then if you are building form a json file where and how is this file generated?

If Twitter changes their API, we are SOL one way or another. the JSON file is produced by my APIHelper.

thedavecarroll commented 5 years ago

@kilasuit @nohwnd @thomasrayner @ChrisLGardner

@mkellerman and I would like to invite you four to an AzureDevOps project for a possible redesign of this module. Let us know if you're interested and we can send you an invite.

This will be the first collaboration project that either of us has worked on, so if you have any suggestions that would be fantastic.

ChrisLGardner commented 5 years ago

I'm happy to help out with that. Feel free to send the invite to whatever email I have attached to github, it's probably got an MSDN linked to it so you should be able to use that license option.


From: Dave Carroll notifications@github.com Sent: Saturday, March 9, 2019 2:18:59 PM To: mkellerman/PSTwitterAPI Cc: Chris Gardner; Mention Subject: Re: [mkellerman/PSTwitterAPI] Remove all _ from function names (#29)

@kilasuithttps://github.com/kilasuit @nohwndhttps://github.com/nohwnd @thomasraynerhttps://github.com/thomasrayner @ChrisLGardnerhttps://github.com/ChrisLGardner

@mkellermanhttps://github.com/mkellerman and I would like to invite you four to an AzureDevOps project for a possible redesign of this module. Let us know if you're interested and we can send you an invite.

This will be the first collaboration project that either of us has worked on, so if you have any suggestions that would be fantastic.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mkellerman/PSTwitterAPI/issues/29#issuecomment-471181685, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQDqFte_LkfPb9MRKwD-cf6VkPpnfvv1ks5vU8LTgaJpZM4agLcq.

kilasuit commented 5 years ago

Is there a need to have it away from GH and not just be a fork here?

mkellerman commented 5 years ago

That was my suggestion :p

thedavecarroll commented 5 years ago

I tried creating a project on here but couldn't link it to a forked repo.

Honestly, I was wanting to learn Azure DevOps and thought it would be a good place to start.

A project board could provide a place for discussion without having to create commits to a repo.

mkellerman commented 5 years ago

It’s a temporary fork, that will either get merged into the main repo, or get canned (or stay it’s own project).

ChrisLGardner commented 5 years ago

Issues work fine for most discussions. I'd figured Azure DevOps would be for the build and release side rather than project management.


From: Marc R Kellerman notifications@github.com Sent: Saturday, March 9, 2019 5:24:07 PM To: mkellerman/PSTwitterAPI Cc: Chris Gardner; Mention Subject: Re: [mkellerman/PSTwitterAPI] Remove all _ from function names (#29)

It’s a temporary fork, that will either get merged into the main repo, or get canned (or stay it’s own project).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mkellerman/PSTwitterAPI/issues/29#issuecomment-471202976, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQDqFptrjXDJfVxxuB_yLN3DBYzAEM03ks5vU-43gaJpZM4agLcq.

mkellerman commented 5 years ago

AzureDevOps is already used on the main repo for the CI/CD build/test/publish process.

mkellerman commented 5 years ago

I can’t fork my own project lol

kilasuit commented 5 years ago

forked to https://github.com/PowerShellModules/PSTwitterAPI

if you are all on the PS Slack lets chat over there & get you invited into the GH org

mkellerman commented 5 years ago

There is a #PSTwitterAPI channel in discord. Not sure if it’s replicated in Slack

kilasuit commented 5 years ago

There is a #PSTwitterAPI channel in discord. Not sure if it’s replicated in Slack

It is :-)

pauby commented 4 years ago

I came here to express the exact same thoughts. Is there any update on this (other than the forked repo)?

mkellerman commented 4 years ago

No update on this.

I think for now, i might just remove the '_' all together, instead of trying to redesign all the functions with dynamic switches.

Unless someone wants to step up and help design it differently.

pauby commented 4 years ago

I'm actually happy to get involved here. It's been on my backlog to look for a decent Twitter module for a while and I came across yours which does everything I need. It's not just now ... PowerShelly, as has been said 😄

For me I think leaving the original function as is, for backwards compatibility, makes some sense. Unless somebody has another suggestion. But also putting together functions based on functionality rather than slaving them to the API 'names' makes a lot of sense too.

It's something I'd like to take the time to look at and submit some PR's. I'm assuming they would be submitted here rather than the original repo?

thedavecarroll commented 4 years ago

@pauby, I had created some issues in a private forked repo that addressed some design issues. I don't know how to add you to that.

Also, a Slack channel was created for this module as well. Are you on the Slack PowerShell workspace?

pauby commented 4 years ago

@thedavecarroll You should be able to add me to that in the Settings of the repo.

I am on PowerShell Slack. You can find me as pauby.

mkellerman commented 4 years ago

Anyone has an update on this item? Did someone come up with a better solution?

pauby commented 4 years ago

I keep scheduling time to work on this and get lost in something else. That's not reporting progress but it is reporting that I've not forgotten about this!

kilasuit commented 4 years ago

Looking at it surely the code you use to autogen in here just needs a small amendment https://github.com/mkellerman/PSTwitterAPI/blob/953070cd0e344afb8811c83f967e5f644275f11c/APIHelper/New-APIHelperFunction.ps1#L9-L13

possibly only needing line 11 changed to the below

 $ResourceName = $ResourceNames[0] + ($ResourceNames[1..999] -Join "" -Replace ":", "")

But this is a guess from a quick scan and not testing the code so I could be wrong

kilasuit commented 4 years ago

Though perphas the auto gen creates private functions and then work is done to create better user friendly functions like as per this comment https://github.com/mkellerman/PSTwitterAPI/issues/29#issuecomment-460096701

In time you could even auto gen those commands using a lookup table but that seems like overkill.

mkellerman commented 4 years ago

Okay, so keeping it simple.. Get-TwitterStatuses_Lookup becomes Get-TwitterStatusesLookup

I’m okay with this change. This is definitely a break change to anyone who’s using this module current. Should I keep the old name as an alias?

pauby commented 4 years ago

My understanding, like @kilasuit mentioned, is that this all needs rewritten. We can use the code that is there but the function names need to reflect what they do rather than Twitter's API.

My 2p - Get-TwitterStatusesLookup is an AWFUL name for a function! 😢

mkellerman commented 4 years ago

But this is a module to interact with an API.. why wouldn’t the function names represent the APIs?

We are better off making a new module: PSTwitter, and have a limited, user-friendly, functions that uses PSTwitterAPI as a dependency.

So everyone is happy lol

pauby commented 4 years ago

But this is a module to interact with an API.. why wouldn’t the function names represent the APIs?

Function names should reflect the task that it performs or the results it brings back. You don't interact with an API for the sake of interacting with an API. You interact with it do get results or perform a task. Function names should reflect that.

We are better off making a new module: PSTwitter, and have a limited, user-friendly, functions that uses PSTwitterAPI as a dependency.

I completely agree. There is nothing that we can do with this particular module that would not break everything for everybody (unless we keep all of the old functions but then the module name would not reflect the new functions, if that makes sense).

pauby commented 4 years ago

Can I suggest a new module name? Twitter, all one word on it's own. We already have PsTwitter and TwitterCmdlets in the Gallery. The consensus now seems to be to have modules without Posh or PS prefixes, hence my choice.

(When I said 'consensus' I did remember reading a thread on this somewhere - please don't ask me where and if you know where it was, or disagree please say so 😅 )

ChrisLGardner commented 4 years ago

Do we care about breaking changes? The module is versioned 0.x so anything goes at that point, preferably avoiding breaking changes but sometimes they happen and people should be pinning versions or reviewing releases before the update (I'm well aware that's not how most people work but they need to learn to do it).

pauby commented 4 years ago

My thoughts around a new module were mainly aimed at the name. I don't think it fits with a module in the way I described it.

But I agree with absolutely everything you said 😄

mkellerman commented 4 years ago

I’m sorry to force the issue guys.. but I personally need to interact with the API in all its shapes and forms.

Due to hundreds of API endpoints, and different ways someone might interact with these endpoints, this module was created to expose all these endpoints in a quick and easy way for a powershell user.

Like I said, let’s create a ‘Twitter’ module, that imports this PSTwitterAPI, and provides a user meaningful name for the few functions most users use/need.

But the point of this module is to interact with the API.

thedavecarroll commented 4 years ago

Being able to script the creation of functions is a great idea, but you must check the source for garbage data.

For instance, just looking at Send-TwitterDirectMessages_IndicateTyping, I see a few minor issues.

Has Send-TwitterCustomProfiles_New.Json been tested?

The function Get-TwitterGeo_Search has attribute:street_address parameter. Attempting to use the parameter will generate an error, but the long and lat parameters work fine.

Get-TwitterGeo_Search -attribute:street_address '1212 My Street'

errors : {@{code=6; message=No data available for the given coordinate.}}
query  : @{url=https://api.twitter.com/1.1/geo/search.json?attribute:street_address=street_address&lat=1212%20My%20Street; type=search; params=}

Get-TwitterGeo_Search -'attribute:street_address' '1212 My Street'  | fl

errors : {@{code=12; message=You must provide valid coordinates, IP address, query, or attributes.}}
query  : @{url=https://api.twitter.com/1.1/geo/search.json?lat=-attribute%3Astreet_address&long=1212%20My%20Street; type=search; params=}

As suggested, a completely new module, whether dependent upon this one or not, would be needed to provide all PowerShell users, from beginners to highly skilled developers, a consistent experience with current best practices.

Personally, I would like to see a PowerShell module for Twitter that would provide or answer the following.

If anyone would like to greenfield a new community repo (with the goal of creating a new module) and establish some guidelines around the list above or other aspects that I didn't cover, I would be glad to assist.

Thanks, Dave

mkellerman commented 4 years ago

Everything you said is right.

I haven’t been able or the need to test out all the automated functions my API helper provided. It’s only there to setup the framework for most of the functions.

I agree that user focused powershell functions should be provided to the end user. So maybe we simply make the API endpoint functions private, and the new functions public?

Is there a way to import-module with a switch to expose ‘all’ the endpoints when we import the module? So we don’t have to maintain two separate modules?

pauby commented 4 years ago

So maybe we simply make the API endpoint functions private, and the new functions public?

You previously said this:

but I personally need to interact with the API in all its shapes and forms.

If you want to use the current module functions then making them private in a new module is not going to work for end users as they won't be able to access them.

My thoughts are, as @thedavecarroll said along with others, that we create a new module (Let's just call it Twitter for the sake of this thread and so we know what we're referring to but I don't care what it's called) and we rewrite from scratch, potentially using code from the PSTwitterAPI module. We've gone back and forth over this a few times, I think we need to draw a line in the sand and agree one way or the other.

On this comment:

I am happy to create that but I think there is likely to be a better place for it in a 'community repo' (again as @thedavecarroll said).

mkellerman commented 4 years ago

If we can make the endpoint functions private/public with a switch when we load the module, this might make things simpler than having 2 separate modules.

Even if it would be an environment variable we need to set prior to loading the module.

This would only be used for a select few users.. as the user friendly functions would give 99% of what the general public needs.

So i vote the rewrite PSTwitterAPI.. keeping the endpoint functions (but without the '_'), and having a new set of user friendly functions that uses the endpoint functions as the backbone.

vexx32 commented 4 years ago

My 2 cents:

pauby commented 4 years ago

If we can make the endpoint functions private/public with a switch when we load the module, this might make things simpler than having 2 separate modules.

I agree with @vexx32 this would be a little weird to do. I don't know any other module that does this and I'm not actually sure of a practical way to do it (disclosure, I've not looked or had any experience doing it as I don't know any other module that does it, as I said).

This would only be used for a select few users.. as the user friendly functions would give 99% of what the general public needs.

I think putting in all the work for a select few users is counter productive. For those users, this module works for them as it stands.

We've only had 3 votes on that last comment (it was only posted yesterday though). However, does anybody have a'community repo' in mind? Failing that I'm going to create it and we can shift it later?

mkellerman commented 4 years ago

We can repurpose this one if this is how the community wants it.

I’m eager to see how this is going to evolve.

I can add collaborators. Who’s in?

pauby commented 4 years ago

I would personally prefer to see it under a community repo rather than a personal one if multiple people are going to work on it.

kilasuit commented 4 years ago

But this is a module to interact with an API.. why wouldn’t the function names represent the APIs?

End user usability - you can always use module scope calling to call the private functions directly like so (pulled from my PesterHelpers module at https://github.com/PowerShellModules/PesterHelpers/blob/master/Public/Export/Export-AllModuleFunction.ps1 - take note of the line

 $AllFunctions = & $moduleData {Param($modulename) (Get-command -CommandType Function -Module $modulename).Where{$_.CommandType -ne 'Cmdlet'}} $module

    $AllFunctions = & $moduleData {Param($modulename) (Get-command -CommandType Function -Module $modulename).Where{$_.CommandType -ne 'Cmdlet'}} $module
    $PrivateFunctions = Compare-Object $PublicFunctions -DifferenceObject $AllFunctions -PassThru -Verbose:$VerbosePreference

    Foreach ($PrivateFunction in $PrivateFunctions){
    Write-Verbose "We Found $($PrivateFunction.Name) that is not being Exported"
    }

    $PublicFunctions | ForEach-Object { Export-Function -Function $_.Name -ResolvedFunction $_ -OutPath $OutPath -Verbose:$VerbosePreference }

    $PrivateFunctions | ForEach-Object { Export-Function -Function $_.Name -ResolvedFunction $_ -OutPath $OutPath -PrivateFunction -Verbose:$VerbosePreference }

We are better off making a new module: PSTwitter, and have a limited, user-friendly, functions that uses PSTwitterAPI as a dependency.

2 places to update = more likelihood one lags behind & PSTwitter already exists

So everyone is happy lol

As seen in this thread I'd see that isn't the case

I would personally prefer to see it under a community repo rather than a personal one if multiple people are going to work on it.

I said the same months (if not over a year ago) and hence it was forked to https://github.com/powershellmodules/PSTwitterAPI/ (which I own and can grant users access to)

mkellerman commented 4 years ago

Listen guys.. I admire what you guys want to do. I think re-writing the module from scratch, as a PS user friendly focus, is fantastic. It's just been a year, and nothing has been done so far by anyone. We are improving this module, and we aren't working on another one. I'm going to close this item. Those of you who want to work on a fresh new module, please continue the conversation on https://github.com/powershellmodules/PSTwitterAPI/

I will continue to update and improve this module in the mean time, as this project will continue to be used by some.