Skip to content

Fix bug to make --clear-output-folder work (solution 1)#1410

Closed
msyyc wants to merge 11 commits intoautorestv3from
fix-clear-output-folder
Closed

Fix bug to make --clear-output-folder work (solution 1)#1410
msyyc wants to merge 11 commits intoautorestv3from
fix-clear-output-folder

Conversation

@msyyc
Copy link
Copy Markdown
Member

@msyyc msyyc commented Aug 29, 2022


output-artifact: python-files
```

Copy link
Copy Markdown
Member Author

@msyyc msyyc Aug 29, 2022

Choose a reason for hiding this comment

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

I don't find effective way to flush the buffer into disk in time. How about changing another way: if we format the file content before write, we can avoid the annoying problem. What's more, we reduce black plugin to make the flow shorter which is cleaner.

Copy link
Copy Markdown
Member Author

@msyyc msyyc Sep 2, 2022

Choose a reason for hiding this comment

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

and it would be breaking to make users not be able to turn off black
@iscai-msft users can still turn off black: https://github.com/Azure/autorest.python/pull/1410/files#r961250499 and https://github.com/Azure/autorest.python/pull/1410/files#r961250788 which will not bring breaking change. And multiapi/multiclient can also use black after this PR.

@msyyc msyyc force-pushed the fix-clear-output-folder branch from 00e5a37 to 1c14ce1 Compare August 31, 2022 05:18
@msyyc msyyc force-pushed the fix-clear-output-folder branch from ca3fd9d to 16eb4d2 Compare August 31, 2022 06:22
@msyyc msyyc marked this pull request as ready for review August 31, 2022 06:34
Comment thread packages/autorest.python/README.md
"Loading python.json file. This behavior will be depreacted"
)
self.options.update(python_json)
self._black = python_json.get("black", True)
Copy link
Copy Markdown
Member Author

@msyyc msyyc Sep 2, 2022

Choose a reason for hiding this comment

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

users can still turn off black (cadl-python)

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.

@msyyc is there really no way to keep black as a separate plugin? Conceptually it really is a plugin, so it makes the code a lot less clean by adding black throughout the different plugins. I can take a look in a bit to see if it can't be done, but I think there should be a way to fix the current plugin we have, instead of doing this refactoring

Copy link
Copy Markdown
Member Author

@msyyc msyyc Sep 7, 2022

Choose a reason for hiding this comment

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

@iscai-msft It is nice if you can take a look. stream.flush can't ensure flush to disk before black read it. The memory inconsistency is always hard to handle so I think the best way is to bypass it completely. Plugin black may be clean but it can't work for multiapi/multiclient. In one word, my fix pr is just one of possible ways but I think it can resolve the problem for good.
nit: the bug seriously influence our release for mgmt SDK so I hope it can be resolved asap.
add @lmazuel for awareness.

) -> None:
super().__init__(output_folder=output_folder)
self._autorestapi = autorestapi
self._black = autorestapi.get_boolean_value("black", True)
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.

users can still turn off black(autorest)

@Azure Azure deleted a comment from iscai-msft Sep 2, 2022
Comment thread packages/autorest.python/ChangeLog.md Outdated
@weidongxu-microsoft
Copy link
Copy Markdown
Member

weidongxu-microsoft commented Sep 6, 2022

What's the reason that many samples/tests files get updated? Is it because previously black did not apply to them due to the bug?

@msyyc
Copy link
Copy Markdown
Member Author

msyyc commented Sep 6, 2022

What's the reason that many samples/tests files get updated? Is it because previously black did not apply to them due to the bug?

Yeah. And with the fix, multiapi/multiclient can also enable black which is not permitted before.

@msyyc msyyc changed the title Fix bug to make --clear-output-folder work Fix bug to make --clear-output-folder work (solution 1) Sep 8, 2022
@msyyc
Copy link
Copy Markdown
Member Author

msyyc commented Sep 15, 2022

close for #1434

@msyyc msyyc closed this Sep 15, 2022
@msyyc msyyc deleted the fix-clear-output-folder branch September 15, 2022 05:28
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.

[Bug] --clear-output-folder does not work for autorest.python > 5.19.0

4 participants