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

[WPB-7222] Drop depencency on convertible-strings in production code #4001

Merged
merged 28 commits into from
Apr 18, 2024

Conversation

mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Apr 15, 2024

The PR removes all usages of the cs function (from ConvertibleStrings) in the application code.

Tracked by https://wearezeta.atlassian.net/browse/WPB-7222.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@mdimjasevic mdimjasevic force-pushed the wpb-7222/drop-cs-from-app-code branch from 7329ed5 to bac2f70 Compare April 16, 2024 08:25
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 16, 2024
@mdimjasevic mdimjasevic force-pushed the wpb-7222/drop-cs-from-app-code branch from defceec to 4251670 Compare April 16, 2024 13:28
@MangoIV MangoIV marked this pull request as ready for review April 17, 2024 08:58
@MangoIV MangoIV requested a review from fisx April 17, 2024 09:38
@MangoIV
Copy link
Contributor

MangoIV commented Apr 17, 2024

this is a tricky PR; a lot of trivial changes but some changes that you have to look at carefully 👀

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

i would really appreciate if you could drop the habit of reformatting so much to your taste, specifically line add line breaks. if we all keep doing that we'll drown in diff noise.

it's probably hard for you because it's done by your tooling? but maybe you can turn it off somehow, or at least dial it down?

just for the future. otherwise this is all fine 👍

services/spar/src/Spar/Scim/Auth.hs Show resolved Hide resolved
@MangoIV
Copy link
Contributor

MangoIV commented Apr 17, 2024

i would really appreciate if you could drop the habit of reformatting so much to your taste, specifically line add line breaks. if we all keep doing that we'll drown in diff noise.

@fisx this wasn't me this time! Marko did all these formatting changes; I actually like them though, it makes the lines a tad shorter. :D

@MangoIV
Copy link
Contributor

MangoIV commented Apr 17, 2024

Argh. Hoogle upload is stuck again

@mdimjasevic
Copy link
Contributor Author

i would really appreciate if you could drop the habit of reformatting so much to your taste, specifically line add line breaks. if we all keep doing that we'll drown in diff noise.

I did this line reformatting and I disagree with what you just said: to me this is not diff noise. I either have to use a tiny tiny font size to see the whole 120+ character line I am editing (which is bad for my health) or I have to break it up into smaller chunks so I can see in one screen what I am editing.

Also, you can configure the GitHub web UI to ignore white space diff when reviewing, which will highlight the actual changes.

Let me know if you know of a way how to edit this in a manageable way and without breaking up the long lines.

@mdimjasevic mdimjasevic force-pushed the wpb-7222/drop-cs-from-app-code branch from 911800d to 8795a84 Compare April 18, 2024 07:29
@MangoIV
Copy link
Contributor

MangoIV commented Apr 18, 2024

@mdimjasevic i think this was more or less directly to me because I have the habit of doing more or less noisy refactoring on the side and we talked about that and I agreed that it’s bad. So I guess that’s why Matthias reminded me to do that less.

@MangoIV MangoIV merged commit 9fff6f9 into develop Apr 18, 2024
8 checks passed
@MangoIV MangoIV deleted the wpb-7222/drop-cs-from-app-code branch April 18, 2024 08:35
@fisx
Copy link
Contributor

fisx commented Apr 19, 2024

I either...

there is a third choice: set up your editor to support line break. :)

but i get your point. everything anybody does breaks somebody's workflow... https://xkcd.com/1172/

(ignore-whitespace on github doesn't work very well. i suspect it breaks semantics sometimes, but worse, it doesn't catch all the noise that bothers me.)

but i've made my point, and i don't see an easy solution. feel free to ignore me, then.

@MangoIV
Copy link
Contributor

MangoIV commented Apr 19, 2024

out of all things, ormolu decided to not enforce line length but doesn't adhere to one-line formatting anywhere...

@mdimjasevic
Copy link
Contributor Author

there is a third choice: set up your editor to support line break. :)

Do you mean line wrapping?

but i get your point. everything anybody does breaks somebody's workflow... https://xkcd.com/1172/

Fair, it does break my workflow, but I think it's not just mine. We often pair and we screen-share where we always have to increase the font size so the other side can read from their screen, and for these things long lines are just no good.

@fisx
Copy link
Contributor

fisx commented Apr 22, 2024

sorry, yes, line wrapping.

i'll shut up now. :)

@echoes-hq echoes-hq bot added the echoes: technical-debt Changes intended at mitigating risks label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-debt Changes intended at mitigating risks ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants