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

chore: jsonfmt search #6240

Merged
merged 1 commit into from
Jun 10, 2019
Merged

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Jun 6, 2019

Ran jsonfmt -w "specification/search/**/*.json"

Ran `jsonfmt -w "specification/search/**/*.json"`
@nschonni nschonni requested a review from brjohnstmsft as a code owner June 6, 2019 05:11
@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jun 6, 2019

SDK Automation [Logs] (Generated from 37cb49a)

Pending Python: Azure/azure-sdk-for-python
  • Package generation pending.
Pending Java: Azure/azure-sdk-for-java
  • Package generation pending.
Pending Go: Azure/azure-sdk-for-go
  • Package generation pending.
Pending JavaScript: Azure/azure-sdk-for-js
  • Package generation pending.
Pending Ruby: Azure/azure-sdk-for-ruby
  • Package generation pending.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Jun 6, 2019

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#5703

@AutorestCI
Copy link

AutorestCI commented Jun 6, 2019

Automation for azure-sdk-for-js

A PR has been created for you:
Azure/azure-sdk-for-js#3613

@AutorestCI
Copy link

AutorestCI commented Jun 6, 2019

Automation for azure-sdk-for-java

Encountered a Subprocess error: (azure-sdk-for-java)

Command: ['/usr/local/bin/autorest', '/tmp/tmpd4snmef7/rest/specification/search/data-plane/Microsoft.Azure.Search.Data/readme.md', '--perform-load=false', '--swagger-to-sdk', '--output-artifact=configuration.json', '--input-file=foo', '--output-folder=/tmp/tmp2_gkux8e']
Finished with return code 7
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.12.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Failure:
Error: Unable to start AutoRest Core from /root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core
Error: Unable to start AutoRest Core from /root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core
    at main (/opt/node_modules/autorest/dist/app.js:232:19)
    at <anonymous>

/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist/app.js:33
    autorest_core_1.Shutdown();
    ^
ReferenceError: autorest_core_1 is not defined
    at process.on (/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist/app.js:33:5)
    at emitOne (events.js:121:20)
    at process.emit (events.js:211:7)
    at process.emit (/node_modules/source-map-support/source-map-support.js:439:21)
fs.js:612
  return binding.close(fd);
                 ^

Error: EBADF: bad file descriptor, close
    at Object.fs.closeSync (fs.js:612:18)
    at StaticVolumeFile.shutdown (/opt/node_modules/autorest/dist/static-loader.js:352:10)
    at StaticFilesystem.shutdown (/opt/node_modules/autorest/dist/static-loader.js:406:17)
    at process.exit.n [as exit] (/opt/node_modules/autorest/dist/static-loader.js:169:11)
    at printErrorAndExit (/node_modules/source-map-support/source-map-support.js:423:11)
    at process.emit (/node_modules/source-map-support/source-map-support.js:435:16)
    at process._fatalException (bootstrap_node.js:391:26)

@AutorestCI
Copy link

AutorestCI commented Jun 6, 2019

Automation for azure-sdk-for-ruby

A PR has been created for you:
Azure/azure-sdk-for-ruby#2554

@AutorestCI
Copy link

AutorestCI commented Jun 6, 2019

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@brjohnstmsft
Copy link
Member

@nschonni I have some questions and concerns:

  1. The diffs are too large to review, but I trust there are only cosmetic changes and no functional ones, correct?
  2. What's motivating this change?
  3. What's the plan to keep files from drifting from this format? Having someone periodically run this command and risk merge conflicts with open PRs doesn't seem like a scalable solution.

@nschonni
Copy link
Contributor Author

nschonni commented Jun 6, 2019

1. The diffs are too large to review, but I trust there are only cosmetic changes and no functional ones, correct?

Yup, formatting only. If I run into simple Model Validation fixes in these PRs, I keep them as separate commits

2\. What's motivating this change?

Easier to visually parse (subjective), and most IDEs will try to format in a similar way depending on the developers setup. I believe the "Hub" also now uses this format. Also just random busy work I do late at night when I'm not sleeping

3\. What's the plan to keep files from drifting from this format? Having someone periodically run this command and risk merge conflicts with open PRs doesn't seem like a scalable solution.

Did a POC in #6238 that I'll re-PR after the bulk of these is landed. So far ~70 of these PRs are done, ~40 with open, ~20 folders not done yet

@brjohnstmsft
Copy link
Member

@nschonni What's the proposed flow for using jsonfmt in the build? Would it modify the files being changed as a side-effect, or just cause errors or warnings in the build if they're not properly formatted...?

@nschonni
Copy link
Contributor Author

nschonni commented Jun 6, 2019

In that docker CI build, I used the --fastfail option to exit on the first diff it hits. If you look at the different build statuses in the different commits, it fails the task when there is a diff, and is green if there is no diffs.
Locally I'm running with the -w to update the files in-place though

@brjohnstmsft
Copy link
Member

@nschonni Sounds good. One more question -- is running jsonfmt equivalent to opening a JSON file in VS Code and hitting shift-alt-f? That's going to be the most common scenario, at least for the Microsoft engineers who author and maintain these specs.

@nschonni
Copy link
Contributor Author

nschonni commented Jun 6, 2019

Yeah, i did a quick test. The EditorConfig setting in the repo should make the "Format Document" indent and add the EOF blank the same way.. jsonfmt was just the most straight forward tool i found to do these changes in bulk (and quicker)

@brjohnstmsft
Copy link
Member

@nschonni Cool, thanks for confirming! I've approved the PR.

@sarangan12 Feel free to merge this as it contains only formatting changes.

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.

5 participants