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

[Proposal] Hope to standardize the code #273

Closed
ggdream opened this issue Oct 27, 2021 · 7 comments
Closed

[Proposal] Hope to standardize the code #273

ggdream opened this issue Oct 27, 2021 · 7 comments

Comments

@ggdream
Copy link

ggdream commented Oct 27, 2021

Hello, the offical team.

I am glad that there is such a third-party library in Dart, which is very convenient to help us improve development efficiency.
I am using this library for a small project these days, and it is not good in the process of using it.

First of all, let's see the entry GitHub(auth: xxx). I have to admit that in some other languages (such as Golang), this package is particularly good, but in Dart, it is a bit bad.

  • Account and password authentication methods have been banned by GitHub, so this category should be set to @Deprecated.
  • From the perspective of Dart syntax, it may be better to use selected args and assert in this situation.

Second, in many places, Dart syntax is not used properly, and many of the parameters that should be set for required are selected, which is directly asserted by ! in the source code and made a judgment. E.g:

Future<Repository> editRepository(RepositorySlug slug,
{String? name,
String? description,
String? homepage,
bool? private,
bool? hasIssues,
bool? hasWiki,
bool? hasDownloads}) async {
ArgumentError.checkNotNull(slug);
final data = createNonNullMap({
'name': name!,
'description': description!,
'homepage': homepage!,
'private': private!,
'has_issues': hasIssues!,
'has_wiki': hasWiki!,
'has_downloads': hasDownloads!,
'default_branch': 'defaultBranch'
});

class CreateFile {
CreateFile(
{this.path, this.content, this.message, this.branch, this.committer});
String? path;
String? message;
String? content;
String? branch;
CommitUser? committer;

To be honest, the code is a bit bad. I haven't used GitHub API many times before, so I tried this library first, but because of the code listed above, I couldn't troubleshoot the error. Because if I don't look at the source code, I have no way to know whether you have set default parameters for the original mandatory parameters.

Therefore, I solved the problem by reading the GitHub document and analyzing the network requests issued by DevTools-Network. Speaking of documentation, example may also need to be modified. No matter for native or flutter, the content of dart-web is not needed. It seems to be too strenuous.

Hope to get better and better.

@github-actions
Copy link

👋 Thanks for reporting! @robrbecker will take a look.

@robrbecker
Copy link
Member

Thanks for your feedback. Keep in mind that this project isn't maintained by a team. It's more or less just me as a volunteer who took it over after being only lightly maintained.

After reading your message, it sounds like there are 3 items I can follow up on:

  1. Deprecate or remove username/password authentication
  2. Add dartdoc strings to describe those methods in particular, and more generally to all methods
  3. Better examples in command line use and in flutter

@Pacane
Copy link
Contributor

Pacane commented Oct 27, 2021

Good Lord, you're a patient person @robrbecker Thank you for doing this haha.

@ggdream
Copy link
Author

ggdream commented Oct 28, 2021

Ok, very much looking forward to it

@robrbecker
Copy link
Member

Closing and linked separate issues for the 3 items that came out of this discussion:

remove username/password: #280
flutter example: #219
editRepository dartdocs: #282

@ggdream
Copy link
Author

ggdream commented Nov 30, 2021 via email

@theRealBitcoinClub
Copy link

@ggdream Would you be able to give any advice on my latest comment in: #219 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants