Skip to content

Rename the NGramNgramExtractor class to a better name. #525

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.

Sign up for GitHub

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

Jump to bottom
Closed
sfilipi opened this issue Jul 12, 2018 · 8 comments
Closed

Rename the NGramNgramExtractor class to a better name. #525

sfilipi opened this issue Jul 12, 2018 · 8 comments

Comments

@sfilipi
Copy link
Member

sfilipi commented Jul 12, 2018

I think the NGramNgramExtractor class in the CSharpApi.cs should have a better name.

Circa line 17990:
public sealed class NGramNgramExtractor : NgramExtractor
@sfilipi sfilipi added good first issue Good for newcomers up-for-grabs A good issue to fix if you are trying to contribute to the project labels Jul 12, 2018
@Ivanidzo4ka
Copy link
Contributor

Don't forget about NGramHashNgramExtractor !

@shauheen
Copy link
Contributor

While I agree with this, please keep in mind that #371 may change the mechanics of this completely.

@Ivanidzo4ka
Copy link
Contributor

It's not only entry point API, it's also real names of the classes, which we will expose through proposed by Tom API.

@shauheen
Copy link
Contributor

That is correct, that's why I said the mechanics of it would be changed.

@sarthak268
Copy link

Is this issue already assigned? If not please assign.

@SriRamLakshmiNarasimhan
Copy link

I would like to pick this up as my first good issue.

@justinormont
Copy link
Contributor

Any proposals for new names for NGramNgramExtractor and NGramHashNgramExtractor?

suhailsinghbains pushed a commit to suhailsinghbains/machinelearning that referenced this issue Oct 21, 2018
[Fixes] dotnet#525: Renamed the NGramNgramExtractor class to a better…
0d456e4 
… name
@suhailsinghbains suhailsinghbains mentioned this issue Oct 21, 2018
Closed
@shauheen shauheen added the need info This issue needs more info before triage label Dec 6, 2018
@Ivanidzo4ka Ivanidzo4ka removed good first issue Good for newcomers need info This issue needs more info before triage up-for-grabs A good issue to fix if you are trying to contribute to the project labels Feb 6, 2019
@sfilipi
Copy link
Member Author

sfilipi commented Feb 27, 2019

The C#Api is no longer there, so this is not relevant anymore.

@sfilipi sfilipi closed this as completed Feb 27, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@Ivanidzo4ka @shauheen @justinormont @sfilipi @sarthak268 @SriRamLakshmiNarasimhan