-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(specs): add analytics
specs and client.
#36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Massive PR ! There seems to be a lot of redundancy with the types but there are all uniques, it would be best with generics but impossible with code generation.
content: | ||
application/json: | ||
schema: | ||
oneOf: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java will be sad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to transform this with polymorphism or optional properties ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that it wouldn't represent the API... but indeed for Java (😢) we could have a fully optional type
specs/common/parameters.yml
Outdated
EndDate: | ||
in: query | ||
name: endDate | ||
description: The upper bound timestamp (a date, a string like “2006-01-02”) of the period to analyze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need a dot here ? The linter didn't catch that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yamllint
did not catch it and I think eslint
automatically applied a dot
on the output. I wonder if we could add a spec rule, I'll check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Just small unrelated comments
return ( | ||
isTimedOut || | ||
isNetworkError({ isTimedOut, status }) || | ||
(~~(status / 100) !== 2 && ~~(status / 100) !== 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, I'm sure it was copied from nuno but are we fine those ~~
I honestly don't know what they mean ahah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, It seems like an overcomplicated way to have the absolute value of status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least 4 people said this ahah I think we can remove them they are more confusing than anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will do a second PR right after
🧭 What and Why
🎟 JIRA Ticket:
Changes included:
analytics
specs and clientmax-params
in eslint configeslint
command on fail (see comment)🧪 Test
CI :D