nuclearpasta / react-native-drax

A drag-and-drop system for React Native
MIT License
554 stars 69 forks source link

Add prettier #73

Closed special-character closed 2 years ago

special-character commented 3 years ago

I think it would be a good idea to add prettier to this project so that contributions from different forks don't have conflicts based on styling differences.

Happy to do the setup for this. I can use the rules I normally have in my other projects but I am not very dogmatic about them.

special-character commented 3 years ago

@lafiosca Would you be down for me to do this?

lafiosca commented 3 years ago

Thank you, @special-character. I do not personally use prettier, but I do use eslint and manually adjust my files. I am fine with adding prettier to the library but would prefer for it to adhere to the existing eslint rules I've established for the project. Perhaps the most contentious of these is that I have very specific reasoning for preferring tab indention versus spaces.

special-character commented 3 years ago

@lafiosca That sounds good, I can definitely go back and change the rules to match the eslint. I must have missed some things on my first pass. I will re-submit for review when I fix it to match the eslint rules.

special-character commented 3 years ago

Went back over this and there are some eslint options that conflict with prettier. I am not sure how important these eslint rules are to you but prettier won't be able to adhere to them.

operator-linebreak
object-curly-newline
no-confusing-arrow
implicit-arrow-linebreak
@typescript-eslint/indent
no-mixed-spaces-and-tabs

I need to find out why the prettier formats two places with spaces instead of tabs (even though I set useTabs: true in the prettier config).

Ultimately, I think we have two options: 1) If any of these is a deal breaker, we can close this issue and ditch the PR 2) Remove those specific rules from eslint 3) Add integrations for prettier with eslint-config-prettier and tslint-config-prettier (https://prettier.io/docs/en/integrating-with-linters.html)

lafiosca commented 3 years ago

Thanks, I'll have to check out these rules in more detail when I get a chance and see what the impact on the formatting will be.

lafiosca commented 3 years ago

Now that I've had a chance to review these rules: if prettier is going to intentionally reformat the code to be in opposition to the current eslint settings for those values, I am not a huge fan of the idea 😬 When you say it won't be able to adhere to them, do you mean it will allow code to pass through in violation of those rules, or will it actively be outputting code that breaks the rules?

special-character commented 3 years ago

It will actively change code to break those rules. i.e. prettier would not follow object-curly-newline and format it so the braces don't go on the new line.

special-character commented 3 years ago

We can probably close this if you aren't a fan of breaking those rules. We have our own fork we are maintaining that we can update to do this.

lafiosca commented 3 years ago

To be honest, I inherited most of these style decisions from using the airbnb-typescript eslint rules, with a handful of tweaks my team made. But we have been using them for years now, and the changes seem a bit more jarring than I had anticipated. It pains me that maintaining your own fork has become necessary (not just from this, but likely from my general lack of maintenance availability), but I agree we should probably close this at least for now.

lafiosca commented 3 years ago

I am exploring prettier in another project, and I think it's likely I will revisit this and explore incorporating prettier, husky, and lint-staged into this project. I think the key to making this work is https://nicedoc.io/prettier/eslint-config-prettier