Skip to content
This repository was archived by the owner on Jun 24, 2025. It is now read-only.

Conversation

@pano9000
Copy link
Contributor

@pano9000 pano9000 commented Feb 26, 2025

Hi,

I've ran prettier with our current config and have commited most of the changes it wanted to fix.
I went through each file individually and left out a couple of changes:
A dozen of the proposed changes IMHO were making the code readability worse – I'll follow up on these with a separate PR that will either
a) check if we can finetune the config a tiny bit
or
b) add some //prettier-ignore comments

:-)

Copy link
Contributor

Choose a reason for hiding this comment

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

The doc notes should most certainly be ignored from the linter, since they are automatically generated and will be replaced anyway as soon they are updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point will add the whole folder to .prettierignore then

}

const HIDDEN_ATTRIBUTES = [ "originalFileName", "fileSize", "template", "inherit", "cssClass", "iconClass", "pageSize", "viewType", "geolocation", "docName" ];
const HIDDEN_ATTRIBUTES = ["originalFileName", "fileSize", "template", "inherit", "cssClass", "iconClass", "pageSize", "viewType", "geolocation", "docName"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the spaces look nice, can we alter the rules to add them in?

Comment on lines 778 to 793
ajax?:
| {
/**
* HTTP Method (default: 'GET')
*/
type: string;
/**
* false: Append random '_' argument to the request url to prevent caching.
*/
cache: boolean;
/**
* Default 'json' -> Expect json format and pass json object to callbacks.
*/
dataType: string;
}
| undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule feels kind of exaggerated, to be honest. 🤔

Comment on lines 9 to 16
type Response =
| {
success: true;
}
| {
success: false;
message: string;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, original looks better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I prefer adding a space before and after a class, looks much more readable

@eliandoran eliandoran marked this pull request as draft February 26, 2025 15:32
@pano9000
Copy link
Contributor Author

pano9000 commented Feb 27, 2025

as discussed on Element yesterday:

Unfortunately prettier has some strong opinions which, I also don't tend to agree with always :-D
The problem is, there is not too much they allow to configure (e.g. you cannot selectively disable certain rules ­– otherwise I would've done that with the way they format ternary expressions :-D)

These are the only configuration options:
https://prettier.io/docs/options

I think we have two (or three) options here:

for now I will edit the PR to address the issues above, so that we at least have some formatting going on already – and for the future we'll need to check regarding above options.

Will update the PR by tomorrow evening likely

@pano9000
Copy link
Contributor Author

pano9000 commented Mar 1, 2025

edit: oops - ignore that I read the date wrong :-)

@pano9000 pano9000 marked this pull request as ready for review March 2, 2025 19:48
@pano9000
Copy link
Contributor Author

pano9000 commented Mar 2, 2025

@eliandoran
I've reverted the changes you mentioned above – let's get this merged for now.
Afterwards I will take a look at dprint → because prettier will not allow any configuration to allow the style you prefer (which I do too, to be honest) (and I don't want to prepend every single class with a "prettier-ignore" :-D)

@eliandoran eliandoran merged commit ef9eebc into develop Mar 3, 2025
5 checks passed
@eliandoran eliandoran deleted the chore_prettier branch March 3, 2025 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants