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

add dedent #141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add dedent #141

wants to merge 1 commit into from

Conversation

mjuksel
Copy link
Contributor

@mjuksel mjuksel commented Dec 22, 2023

No description provided.

@gustavoguichard
Copy link
Owner

Hi @mjuksel ! Thanks for the new PR!
I like the overall code but I feel like it could follow our standards a lil more, by reusing internal functions that are already available such as trim, replace, slice, split, etc.

Did you find any barrier on using those?

@mjuksel
Copy link
Contributor Author

mjuksel commented Dec 29, 2023

Did you find any barrier on using those?

@gustavoguichard I kind of did yeah.

  • Trim doesn't take an argument, so it can't remove new lines.
  • Using Split<data, '\n'> removes the trailing empty line for some reason?
  • Because of that, join keeps a leading empty line.
  • Can't filter out leading/trailing empty lines from the split data, as it will remove empty lines everywhere.

So either we should modify the existing types/functions a bit, or use it this way 😄

@gustavoguichard
Copy link
Owner

Interesting, lemme think about it... maybe we are seeing some weaknesses of the other methods.
cc: @jly36963

@mjuksel
Copy link
Contributor Author

mjuksel commented Dec 29, 2023

Interesting, lemme think about it... maybe we are seeing some weaknesses of the other methods.

Right.

Trim type can just have an argument added, but that means we can't use the native trim method.
That can easily be solved by replacing it with the one I have here, or something similar.

I'm assuming most of them are easy fixes/tweaks!

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

2 participants