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

WIP - Updating name and documentation for Transparent HelloWorld Service #189

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

frisso
Copy link
Contributor

@frisso frisso commented Jul 25, 2019

Added documentation, useful for new developers.
It would be nice to shorten the name, so that we can avoid typing "transparenthelloworld" such as in
polycubectl transparenthelloworld add trhw0
but I'm unable to do so right now.

@frisso frisso requested a review from a team as a code owner July 25, 2019 16:28
Copy link
Contributor

@goldenrye goldenrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tr-helloworld looks more aesthetic than trhelloworld IMO.

@frisso
Copy link
Contributor Author

frisso commented Jul 26, 2019

Let's try; I remember something like the "-" character creates troubles with the code generation :-(

@goldenrye
Copy link
Contributor

Let's try; I remember something like the "-" character creates troubles with the code generation :-(

Yes looks swagger auto-gem compiler has problem to generate correct code if there is "-" character, the character following the "-" will be used to generate variable but some place capital some place small-case and code won't be compiled.

@@ -23,7 +23,7 @@ module transparenthelloworld {
enum SLOWPATH;
}
default PASS;
description "Action performed on ingress packets";
description "Action performed on ingress packets (DROP/PASS/SLOWPATH; default: PASS)";
Copy link
Contributor

@mauriciovasquezbernal mauriciovasquezbernal Jul 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think information about possible values and default should not be part of the description.
Maybe the help generation could be improved to include this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right point, Mauricio. Moved now in the "example" section of the YANG. However, the example is not printed on screen by the help, so we should probably modify the command line to include that as well.

@mauriciovasquezbernal
Copy link
Contributor

Few comments:

  • - causes problems in the code generation. Infact the duplicated file you removed because of this.
  • I don't know if this is a good idea to change the name on the datamodel without regenerating the code.
  • Long names should not be a problem with bash autocompletion, you don't have to type all of them.

@frisso frisso force-pushed the frisso-trhw-fixes branch 2 times, most recently from 6edf5aa to 72d0300 Compare July 27, 2019 09:44
@frisso
Copy link
Contributor Author

frisso commented Jul 27, 2019

  • Long names should not be a problem with bash autocompletion, you don't have to type all of them.

Unfortunately this is not true. Long names are consuming a lot of space on the screen.
Type for instance polycubectl -h and see how columns are large, due to the big name we have.
So, I feel we should use shorter names for better visibility.

@mauriciovasquezbernal
Copy link
Contributor

  • Long names should not be a problem with bash autocompletion, you don't have to type all of them.

Unfortunately this is not true. Long names are consuming a lot of space on the screen.
Type for instance polycubectl -h and see how columns are large, due to the big name we have.
So, I feel we should use shorter names for better visibility.

I soft agree with that, just find a name that is shorter but still meaningful, I think tr-helloworld is a quite good one as suggested above.

@frisso frisso force-pushed the frisso-trhw-fixes branch from 72d0300 to b8eff81 Compare July 28, 2019 08:18
Copy link
Contributor

@acloudiator acloudiator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@frisso frisso changed the title Updating name and documentation for Transparent HelloWorld Service WIP - Updating name and documentation for Transparent HelloWorld Service Oct 29, 2019
@acloudiator acloudiator added this to the v.0.11.0 milestone Oct 4, 2020
@acloudiator acloudiator requested a review from a team October 4, 2020 21:40
@acloudiator acloudiator changed the base branch from master to release/v0.11.0 November 4, 2020 17:05
@frisso frisso marked this pull request as draft May 11, 2021 07:05
@frisso frisso changed the base branch from release/v0.11.0 to master June 3, 2021 16:07
@frisso frisso removed this from the v.0.11.0 milestone Jun 3, 2021
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 this pull request may close these issues.

4 participants