-
Notifications
You must be signed in to change notification settings - Fork 6
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(aggregation-api)!: add new endpoint for economy/mc-all
aggregation
#2092
Conversation
8c238fb
to
59f1349
Compare
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.
I don't see the part concerning the regex for column names (see JIRA issue).
You should add and test it.
Also, for mc-all
, there are new type of files that can be requested (see JIRA issue) so you need to implement that.
59f1349
to
bb87d73
Compare
4c272cf
to
a64eb0a
Compare
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.
I didn't read everything but overall the code is super complex and it's really hard to understand your logic so could you add some doc inside the code to make it more readable and maintainable.
0b81083
to
93a59a6
Compare
bce3afa
to
1d0b944
Compare
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.
We must not merge this before making sure the API breaking change is intended and documented in the changelog.
…puts to `STA-mini`
…ve case sensitiveness
…ve case sensitiveness
…Args` readability for `PyCharm`
…s/aggregate/mc-all`
…is raised when relevant
1d0b944
to
8416433
Compare
economy/mc-all
aggregationeconomy/mc-all
aggregation
Context:
for the aggregation
apis
, we want to add additional endpoints that perform aggregation on the foldereconomy/mc-all