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

Records and suggestions from the development of v0.2 #213

Closed
kdada opened this issue Jun 13, 2018 · 5 comments
Closed

Records and suggestions from the development of v0.2 #213

kdada opened this issue Jun 13, 2018 · 5 comments
Assignees
Labels
area/cli Command tool related functions. area/codegen kind/enhancement Categorizes issue or PR as related to enhancement. kind/feature Categorizes issue or PR as related to a new feature. priority/P1 Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@kdada
Copy link
Collaborator

kdada commented Jun 13, 2018

This issue is for recording the rest works from the development of v0.2.

@ddysher

  • Add a README.md file with short introductions about the generated project (the purpose of this readme file is to guide users to create their own README file)
  • Add docs.go for each package
  • How to handle pkg/database
  • glog exits with 255, C/C++ uses -1, which ended up with 255 also
  • Auto this method hard-code quite a few things, which are actually user-facing (part of the framework API)?
  • I'm a little concerned about using init() to register handlers, is there a better alternative?
  • why do you redefine everything here again?.... this is not maintainable
  • are u sure these are the right things to do....
  • Also, i suggest we can make /path/to/apis optional in nirvana api (set the default to pkg/api as we already have a recommended structure.
  • PS. let's change pkg/api to pkg/apis, since we mostly always have more than 1 API in a project :)
  • I'll send a patch to update README template shortly.

@caitong93

  • It is better to split out these templates to dedicated files for better management. Also we should provide a way to customize the default templates.

@zjj2wry

  • add default value to &apiOptions{}, It's easier to add unit tests
  • init should top of api command in help.
  • nirvana init should be have some prompt when init success.
  • would be better hava log like kubectl get node -v 10 in REST.
  • If you do not specify args, you should use the default path, I think you can add a line of log to tell the user.
  • Since it will use the first configuration that can be read, I think it is possible to limit the length of args to not greater than 1.
  • would be better print what dir you read.

Related PRs:
#209
#211
#212

@kdada kdada added kind/feature Categorizes issue or PR as related to a new feature. kind/enhancement Categorizes issue or PR as related to enhancement. priority/P1 Important over the long term, but may not be staffed and/or may need multiple releases to complete. area/codegen area/cli Command tool related functions. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 13, 2018
@kdada kdada added this to the v0.2.0 Alpha milestone Jun 13, 2018
@kdada kdada changed the title Records and improvements from the development of v0.2 Records and suggestions from the development of v0.2 Jun 22, 2018
@kdada
Copy link
Collaborator Author

kdada commented Jun 28, 2018

Add a README.md file with short introductions about the generated project (the purpose of this readme file is to guide users to create their own README file)

Added

Add docs.go for each package

For api servers, doc.go may be not useful. In most scenarios, we don't add a separated file to write docs.

How to handle pkg/database

pkg/database is an irrelevant part with nirvana. We can regard it as a best practice and put it into manuals.

glog exits with 255, C/C++ uses -1, which ended up with 255 also

I just followed golang log package.

Auto this method hard-code quite a few things, which are actually user-facing (part of the framework API)?

I have already recorded it into manuals.

I'm a little concerned about using init() to register handlers, is there a better alternative?

I think init() is a better way than others. In a go file, init only register APIs which are in current file. That means we don't impact other files when we want to add/modify/delete APIs in a file.

why do you redefine everything here again?.... this is not maintainable

The two structs are not completely same. The struct in utils/api contains more fields than the corresponded one in definition (Also some fields are different). We may consider about how to merge them in the future.

are u sure these are the right things to do....

Yes. Ignore some checks in gometalinter for using unsafe package.

Also, i suggest we can make /path/to/apis optional in nirvana api (set the default to pkg/api as we already have a recommended structure.

It's reasonable.

PS. let's change pkg/api to pkg/apis, since we mostly always have more than 1 API in a project :)

It's reasonable.

I'll send a patch to update README template shortly.

OK.

SUMMARY

  1. Add best practice about pkg/database into manuals.
  2. Add best practice about client generation into manuals
  3. Add default path to nirvana api
  4. Rename pkg/api to pkg/apis
  5. Refine README.md from nirvana init

@ddysher WDYT?

@kdada
Copy link
Collaborator Author

kdada commented Jun 28, 2018

It is better to split out these templates to dedicated files for better management. Also we should provide a way to customize the default templates.

It's reasonable but not urgent.

@caitong93 you can refactor it if you are free.

@kdada
Copy link
Collaborator Author

kdada commented Jun 28, 2018

add default value to &apiOptions{}, It's easier to add unit tests

Default values for apiOptions can't make you easy to add unit tests. Because you always need to construct a test project.

init should top of api command in help.

It's default order of cobra. If you thinks it's not Intuitive, you can create a pr to fix it.

nirvana init should be have some prompt when init success.

It's reasonable.

would be better hava log like kubectl get node -v 10 in REST.

rest package supports RequestExecutor. you can write log by yourself.

If you do not specify args, you should use the default path, I think you can add a line of log to tell the user.

Same as

Also, i suggest we can make /path/to/apis optional in nirvana api (set the default to pkg/api as we already have a recommended structure.

Since it will use the first configuration that can be read, I think it is possible to limit the length of args to not greater than 1.

No. In most cases, one arg for path is enough. But sometimes, our descriptors and modifiers may be in different directories. So we need support multiple args.

would be better print what dir you read.

It's reasonable.

** SUMMARY **

  1. Add success message for commands
  2. Print more useful messages when a command is running

@zjj2wry WDYT?

@kdada
Copy link
Collaborator Author

kdada commented Jun 28, 2018

Backlogs:

  1. [P0] Add default path to nirvana api feat(cmd): move /pkg/api to /pkg/apis #221
  2. [P0] Rename pkg/api to pkg/apis feat(cmd): move /pkg/api to /pkg/apis #221
  3. [P0] Remove contention from profiling fix(profiling): remove contention #223
  4. [P0] Add handlers for uploading/downloading static files feat(service): add helper functions to support static file server #222
  5. [P0] Support interface in client generation. fix(golang): support to generate io.Reader and io.ReadCloser #224
  6. [P1] Add best practice about client generation to manuals feat(manuals): add manuals of plugins and clientset #226
  7. [P1] Add success message for commands feat(cmd): move /pkg/api to /pkg/apis #221
  8. [P1] Print more useful messages when a command is running feat(cmd): move /pkg/api to /pkg/apis #221
  9. [P2] Refine README.md from nirvana init @ddysher
  10. [P2] Add best practice about pkg/database to manuals.
  11. [P2] Add more details to manuals

@kdada
Copy link
Collaborator Author

kdada commented Jul 6, 2018

PR has been merged. And we can track the doc by #227.

@kdada kdada closed this as completed Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Command tool related functions. area/codegen kind/enhancement Categorizes issue or PR as related to enhancement. kind/feature Categorizes issue or PR as related to a new feature. priority/P1 Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

4 participants