camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
38 stars 60 forks source link

Modify create session url name and add a complimentary description for qos in api definition #149

Closed ShutingQing closed 1 year ago

ShutingQing commented 1 year ago

What type of PR is this?

What this PR does / why we need it:

From developer perspective, when i look into the api definitions, i may have some suggestions.

  1. For the create session url, currently it's '/sessions' why not use ‘createSessions’ directly?How do you guys feel?
  2. For the QoS profile, through it's been clarified very clearly in documentations, but i'll suggest a brief description of qos_e qos_l qos_m qos_s is required. So people can easily know.
  3. Delete useless .gitignore file.

Which issue(s) this PR fixes:

Fixes #

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs
hdamker commented 1 year ago

@ShutingQing Thanks a lot for your first contribution within QoD!

It is a best practice to split different topics into different issues/PRs, at least into single commits.

Here we have a mix of completely different topics. Also we have the policy to first open an issue, discuss the proposal within the issue and only then open a PR based on the support there (see https://github.com/camaraproject/Governance/blob/main/ProjectStructureAndRoles.md)

In particular:

  • For the create session url, currently it's '/sessions' why not use ‘createSessions’ directly?How do you guys feel?

That would be a major change and would require first a discussion within an issue. But your proposal would be also a violation of the Design Guidelines related to URL definitions, e.g. "verbs are not allowed". "Create" is already given by the http POST verb.

  • For the QoS profile, through it's been clarified very clearly in documentations, but i'll suggest a brief description of qos_e qos_l qos_m qos_s is required. So people can easily know.

That would fit into the PR @jlurien is working on for #144 ... we don't want to have concrete values in the YAML file but some explanation make sense.

  • Delete useless .gitignore file. ok, I will do it as simple housekeeping.

In addition you have added your name to the maintainer file as part of the PR. There is no need to be in the maintainer file for the first contributions. See https://github.com/camaraproject/Governance/blob/main/ProjectStructureAndRoles.md for the definition of maintainers.

ShutingQing commented 1 year ago

Hi @hdamker, thanks for suggestions. I agree it’s good to split different topics, so it will be easier for people to review. I’ll look into your references.

My bad. Before when we contributed in onap, maybe in my project, we didn’t put too much attention on this. Cuz usually the code is a lot. So when we do a code review, we will just go through each line, and leave comments on each line we have questions. And then contributors will make changes again based on review changes till a consensus.