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

Don't call toString on null values #550

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Aug 23, 2021

Connected to scalameta/metals#3054

Copy link
Collaborator

@keynmol keynmol left a comment

Choose a reason for hiding this comment

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

Oof!

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 23, 2021

For Scala 2 we use pprint, so this is not an issue. I wonder if we should actually switch to it for Scala 3 too, but Martin was highly against it.

@tgodzik tgodzik merged commit dd79fac into scalameta:main Aug 23, 2021
@tgodzik tgodzik deleted the fix-null branch August 23, 2021 17:30
@keynmol
Copy link
Collaborator

keynmol commented Aug 23, 2021

but Martin was highly against it

do you have a link to the arguments? pprint in mdoc is unpopular in the cats-effect IO codebases as it deconstructs everything, but I haven't read anything in relation to Scala 3

@tgodzik
Copy link
Contributor Author

tgodzik commented Aug 23, 2021

I think that was discussed in an email, but basically he was against it because this might surprise users, since it worked differently to toString. There is a feature request connected to cats: scalameta/metals-feature-requests#71

We could have an additional setting when to use toString

out.append(nullableToString(value).replace("\n",""))
}

inline private def nullableToString[T](value: T) = {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can be replaced with String.valueOf or an interpolated string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would be equivalent. I can change it later on when doing some work around it. Or do you think this might work more correctly?

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