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.

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

Added description for parameters #127

Merged
merged 14 commits into from
Aug 15, 2019
Merged

Added description for parameters #127

merged 14 commits into from
Aug 15, 2019

Conversation

junaidnz97
Copy link
Contributor

The purpose of this PR is to add parameter description for parameters in the V2 APIs

@junaidnz97 junaidnz97 closed this Aug 8, 2019
@junaidnz97 junaidnz97 reopened this Aug 8, 2019
@jmmut jmmut mentioned this pull request Aug 8, 2019
Merged
@junaidnz97 junaidnz97 changed the title Added description for species,assembly,pageSize,pageNumber parameters Added description for parameters Aug 9, 2019
@junaidnz97 @jmmut
Apply suggestions from code review
795fbef 
Co-Authored-By: jmmut <jomutlo@gmail.com>
cyenyxe
cyenyxe suggested changes Aug 9, 2019
View reviewed changes
Copy link
Contributor

@cyenyxe cyenyxe left a comment

Choose a reason for hiding this comment

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

I personally find the Swagger annotations more clearly if they are not interleaved with the parameter declarations. You can put them at the top, but I think you may also need to annotate the method; this is not necessarily bad, as adding a description of its purpose is useful.

eva-server/src/main/java/uk/ac/ebi/eva/server/ws/GeneWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/GeneWSServerV2.java Outdated
"/v1/meta/species/list/ in the field named 'assemblyCode'.", required = true)
@RequestParam(name = "assembly") String assembly,
@ApiParam(value = "The number of the page that shoulde be displayed. Starts from 0 and is an integer." +
" e.g. 0")
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add this last part because it will be displayed as default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed :)

eva-server/src/main/java/uk/ac/ebi/eva/server/ws/GeneWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/GeneWSServerV2.java Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated
@ApiParam(value = "Retrieve only variants with exactly this consequence type (as stated by Ensembl VEP)")
@RequestParam(name = "annot-ct", required = false) List<String>
consequenceType,
@ApiParam(value = "Retrieve only variants with exactly this Minor Allele Frequency")
Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly? But we accept ranges using some special characters, don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, right, I saw public VariantRepositoryMafFilter(String maf) { super("st.maf", maf); } and assumed that was the mongo document. It's true that we have VariantRepositoryDoubleFilter which allows <, >, =, <=, >=.

junaidnz97 and others added 2 commits August 9, 2019 19:59
@junaidnz97
Apply suggestions from code review
999a913 
Co-Authored-By: Cristina Yenyxe Gonzalez Garcia <cyenyxe@ebi.ac.uk>
@junaidnz97
change page parameter description
cf850c4
jmmut
jmmut requested changes Aug 12, 2019
View reviewed changes
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated
@ApiParam(value = "Retrieve only variants with exactly this consequence type (as stated by Ensembl VEP)")
@RequestParam(name = "annot-ct", required = false) List<String>
consequenceType,
@ApiParam(value = "Retrieve only variants with exactly this Minor Allele Frequency")
Copy link
Contributor

@jmmut jmmut Aug 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

ok, junaid, can you change the descriptions for maf, polyphen and sift to explain the relational operators that you can use, for instance "<=0.05"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed :)

cyenyxe
cyenyxe reviewed Aug 13, 2019
View reviewed changes
Copy link
Contributor

@cyenyxe cyenyxe left a comment

Choose a reason for hiding this comment

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

Not critical, but I think these suggestions would make some descriptions quicker/easier to read.

eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/VariantWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/VariantWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/VariantWSServerV2.java Outdated Show resolved Hide resolved
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/VariantWSServerV2.java Outdated Show resolved Hide resolved
junaidnz97 and others added 2 commits August 13, 2019 15:44
@junaidnz97
Apply suggestions from code review
f8fe56d 
Co-Authored-By: Cristina Yenyxe Gonzalez Garcia <cyenyxe@ebi.ac.uk>
@junaidnz97
Corrected parameter description
4fee4c3
jmmut
jmmut requested changes Aug 15, 2019
View reviewed changes
eva-server/src/main/java/uk/ac/ebi/eva/server/ws/RegionWSServerV2.java Outdated
@RequestParam(name = "annot-ct", required = false) List<String>
consequenceType,
@ApiParam(value = "Minor Allele Frequency. The value of Minor Allele Frequency with one of the " +
"<, <=, =, >, >= symbols preceding it has to be provided. e.g. >=0.5")
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I'm not sure of a good way to explain this concisely. What about

Retrieve only variants whose Minor Allele Frequency is less than (<), less than or equals (<=), greater than (>), greater than or equals (>=) or equals (=) the provided number. e.g. <0.1

applies to polyphen and sift too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed :)

@jmmut jmmut merged commit 19e7215 into EBIvariation:master Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyenyxe cyenyxe cyenyxe requested changes

@jmmut jmmut jmmut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@junaidnz97 @cyenyxe @jmmut