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

New origin: Atlassian Cloud #25

Merged
merged 7 commits into from
Feb 20, 2019
Merged

New origin: Atlassian Cloud #25

merged 7 commits into from
Feb 20, 2019

Conversation

mishok13
Copy link
Contributor

No description provided.

@iri989 iri989 changed the title WiP: New origin: Atlassian Cloud New origin: Atlassian Cloud Feb 20, 2019
@anton-107 anton-107 merged commit ecd45b6 into master Feb 20, 2019
Copy link
Contributor Author

@mishok13 mishok13 left a comment

Choose a reason for hiding this comment

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

Two big things in here that trigger me:

  • code duplication
  • usage of ad-hoc pagination for APIs that have pagination information (at least according to documentation)

let groups = [];
let users = new Map();

// Since Atlassian does not have a proper API for listing all
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reads like an accidental copy of the comment in getJiraData() :) At this point in the source an attentive reader of the code already knows Atlassian API is wonky so you can skip to the just one short sentences saying that we simply iterate through all groups and then pull every user from that group.

// "incomplete" page.
let index = 0;

while (true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have almost identical code on line 48, I would suggest we move this kind of pagination in a separate function. And perhaps we shouldn't do ad-hoc pagination here at all since https://developer.atlassian.com/cloud/confluence/rest/#api-group-get has pagination information in response.


for(const groupName of groups ) {
let index = 0;
while (true) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

@mishok13
Copy link
Contributor Author

Since the pull request is merged I'd prefer to have a separate pull request addressing my comments.

@mishok13 mishok13 deleted the atlassian-origin branch February 21, 2019 08:47
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.

3 participants