Skip to content

[core-http] Fix formatting errors#4863

Merged
ramya0820 merged 3 commits intoAzure:masterfrom
ramya0820:sb-atom-format-files
Sep 5, 2019
Merged

[core-http] Fix formatting errors#4863
ramya0820 merged 3 commits intoAzure:masterfrom
ramya0820:sb-atom-format-files

Conversation

@ramya0820
Copy link
Copy Markdown
Member

Fir formatting issues to prevent noise later.

@ramya0820 ramya0820 requested a review from daviwil as a code owner August 22, 2019 21:00
@ramya0820 ramya0820 self-assigned this Aug 22, 2019
@ramya0820 ramya0820 requested a review from ramya-rao-a August 22, 2019 21:01
Copy link
Copy Markdown
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

There are 2 places where the formatter changes makes the code very hard to read.
With a simple pulling of JSON.stringify out into a constant can improve this.
Please make the change in the 2 places.

obj,
undefined,
2
)} is not a valid object that can be ` + `enumerated to provide its values as an array.`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is very hard to read. Please pull JSON.stringify(obj, undefined, 2) out into a constant instead of using it inline. This way the formatter should treat it more kindly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, this PR is meant to only fix formatting issues.
Lets do refactoring for readability after the end of main implementation (Atom management API)

Copy link
Copy Markdown
Contributor

@ramya-rao-a ramya-rao-a Aug 25, 2019

Choose a reason for hiding this comment

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

Yes, I understand this PR is to fix the formatting so that further PRs would not have the noise and I appreciate the effort.

As you know, formatting tools exist to make it easier for us to maintain coding styles across all files. If at any point, they cause more harm than good, then we should try our best to fix that especially if the fix is very trivial.

This is one such case.

I would prefer to have this fixed now rather than have this PR introduce a new problem and then have another PR to fix it.

Also, there is a high chance that the implementation for the management apis might not touch this line of code and you might want that PR to be restricted to code related to that feature.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is the concern with use of JSON.stringify(), or this specific string?
'prettier' formatting is to help with readability and I didn't find this that hard to read too.
It can be pulled out - but this would be a general refactoring effort since JSON.stringify() is going to be used in other parts of implementation as well. So we'll likely need to pull this out elsewhere and not just out within this file.
Let's address this after the implementation is in?
I was hoping to spend less time / review cycles on these as it seems too early to address this.
The purpose was that subsequent changes surfaced in PRs are not related to these, and so this can help bring focus to the implementation first.
Overall, JSON.stringify() will likely be used in multiple places and seems a code cleanup detail which can be addressed at end.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is absolutely no problem in the usage of JSON.stringify(). No wide scale refactoring is needed.
The suggestion was to change

throw new Error(
`The provided object ${JSON.stringify(
        obj,
        undefined,
        2
      )} is not a valid object that can be ` + `enumerated to provide its values as an array.`
);

to

const stringifiedObj = JSON.stringify(obj, undefined, 2);
throw new Error(
      `The provided object ${stringifiedObj} is not a valid object that can be ` +
        `enumerated to provide its values as an array.`
    );

pathParameters,
undefined,
2
)}.` +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is very hard to read. Please pull JSON.stringify(pathParameters, undefined, 2) out into a constant instead of using it inline. This way the formatter should treat it more kindly

@ramya-rao-a ramya-rao-a removed the request for review from daviwil August 27, 2019 00:50
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.

2 participants