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

Dependency manager/nuget: use enabled feeds only #13312

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Jun 16, 2022

Prevents Nuget dependency manager trying to access disabled feeds, accessing which could cause errors.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Cool

thanks

@KevinRansom
Copy link
Member

@auduchinok , looks like a formatting error mate.

@auduchinok
Copy link
Member Author

@auduchinok , looks like a formatting error mate.

This doesn't seem right to me, as the code was manually formatted in a way that is allowed by the official style guide.

It's good that the style guide allows variations that help to convey the meaning by emphasizing parts of the code that can be done by carefully placing new lines and empty lines, adding more indent or not and so on, at it may be very important on difficult parts of a codebase. It seems requiring all contributors to automatically format their changes in a way that often lose this formatting makes things a bit worse for both the codebase and the contributors who may have part of their work removed. Ideally, Fantomas should be a bit more flexible and keep the formatting where it's allowed by the style guide.

cc @dsyme @nojaf @vzarytovskii

@smoothdeveloper
Copy link
Contributor

To me, there is no one true formatting, and being over sensitive to it creates more waste than it improves codebase soundness.

Formatting tools and conventions are good guidance and of good help, but there are as many writing styles as there are authors, and reacting strongly against one or other is missing the point that the code can't fit taste of everyone.

If there are formatting comments to be made, the new formatting should be proposed, patching the PR, ideally.

If there is one standard to be enforced everywhere, it should be a CI failure.

We can't go from nothing to all, and we should strike right balance (all is not, the same as nothing is not), and rely on appreciation of contributors more than stringent rules.

The only way for having same consistency would be to take most efficient F# contributor, clone that person, and allow no one else to contribute 🙂.

Please ignore if this goes against better compiler and more contributions.

@vzarytovskii vzarytovskii merged commit f99d851 into dotnet:main Jun 17, 2022
@dsyme
Copy link
Contributor

dsyme commented Jun 20, 2022

@auduchinok We have quite a long setting for fsharp_max_if_then_else_short_width here: https://github.com/dotnet/fsharp/blob/main/.editorconfig#L8

I discussed with @nojaf about suppressing if expr then expr formatting on single line, see fsprojects/fantomas#2299

@auduchinok
Copy link
Member Author

auduchinok commented Jun 20, 2022

@dsyme I'm sorry, my comment wasn't about a particular Fantomas setting, it's about requiring all contributors to have their changes formatted in a way that currently may lose parts of the formatting that is allowed and that may make the code easier to read. After working on tooling and seeing how different people perspectives can be, it just doesn't feel right to me, as we always try to make things as flexible as the language we make a tool for is.

@auduchinok
Copy link
Member Author

A similar example was discussed here: fsprojects/fantomas#2280 (comment)

It's not about assignment or if expressions in particular, it's about preventing a formatter from enforcing a single style in cases where different styles are possible and allowed and when some other style may be preferred in a particular situation (and was explicitly used by a user in the code being formatted).

@auduchinok auduchinok deleted the dependencyManager-disabledFeeds branch June 20, 2022 11:15
@dsyme
Copy link
Contributor

dsyme commented Jun 20, 2022

OK, well, I think I see. In any case, yes, we're giving over formatting preferences to Fantomas and the style guide, within the settings available. Contributors shouldn't try to deliver code readability by jazz formatting (that is, manually choosing between various formattings) except within the limited range of formatting choices preserved by Fantomas (e.g. Fantomas generally respects the adhoc insertion of extra blank lines)

It takes a bit of getting used to.

@nojaf
Copy link
Contributor

nojaf commented Jun 21, 2022

as the code was manually formatted in a way that is allowed by the official style guide.

Your input isn't being checked against the style guide. It is being checked against how Fantomas would have formatted it, based on its understanding of the style guide.

On a side note, if the style guide changes, someone needs to update Fantomas as well in order for it to adhere to the new guide. People seem to think this happens automagically and overnight.

It takes a bit of getting used to.

@auduchinok
Copy link
Member Author

auduchinok commented Jun 21, 2022

Your input isn't being checked against the style guide. It is being checked against how Fantomas would have formatted it, based on its understanding of the style guide.

The point is Fantomas should produce identical code if it's allowed by style guide and settings, like it's done with empty lines.

@nojaf
Copy link
Contributor

nojaf commented Jun 21, 2022

That is how you would like the tool to work, but has never been the mission statement.

@smoothdeveloper
Copy link
Contributor

Seeing https://github.com/dotnet/fsharp/blob/main/DEVGUIDE.md#automated-source-code-formatting and that there is already a CI hook, also learning about "jazz formatting" which maintainers want to prevent, unless it is preserved by Fantomas; it makes most of my comment above moot.

It seems Fantomas is the only way the style guide can evolve, and that Fantomas tries to remove the divergences with style guide in the interim.

Isn't there going to be hurdle with some of the codebase? For example, that which is relying on whitespace alignment and all which is related to trivia (comments which tend to be rejigged by Fantomas IIRC)?

@dsyme
Copy link
Contributor

dsyme commented Jun 21, 2022

@smoothdeveloper Yes, it's possible we will decide not to apply Fantomas to a few select files (ValFlags in TypedTree.fs being one example)

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.

None yet

6 participants