gadicc / node-yahoo-finance2

Unofficial API for Yahoo Finance
https://www.npmjs.com/package/yahoo-finance2
MIT License
394 stars 62 forks source link

Discussion: Linting and Developer Experience #22

Closed pudgereyem closed 3 years ago

pudgereyem commented 3 years ago

Hey @gadicc, I wanted to discuss the linting that's setup for the project, maybe it can be improved.

I can see that:

However, it doesn't do much on my end. I'm using Visual Studio Code. Does it work on your end @gadicc ?

advaiyalad commented 3 years ago

Yeah... for some reason, everything is being converted to double quotes on save, though my formatter is configured to use the .editorconfig and to use single quotes. Using Visual Studio Code.

gadicc commented 3 years ago

Hi, thanks all, will take a look at this tomorrow. I'm using Atom though.

pudgereyem commented 3 years ago

@PythonCreator27 I'm usually using Prettier for code formatting, which caused my code editor to format on save. What I had to do was to disable Prettier for projects that doesn't have a Prettier config. Might not be the same in your case, but thought it might help.

advaiyalad commented 3 years ago

@pudgereyem Sure, I can try disabling it. I am thinking of just adding a .vscode/settings.json and adding "prettier.enable": false to it until a prettier config or some other formatter config is created. Would that work out? Or should I create a prettier config based on the current state of the files?

pudgereyem commented 3 years ago

Hey @PythonCreator27 , if you have the Prettier plugin installed you can just set "Require config" to true. Then Prettier will only run for projects that have a "Prettier config"-file, which this project does not.

image

gadicc commented 3 years ago

This is going to take a while :sweat_smile: A few updates:

Originally I added prettier, but the lack of configuration options became a problem. I understand their philosophy, but, for example, things like:

interface {
  price: number;     // 12.12
  currency: string;  // "USD"
  date: Date;        // 1231232131
}

becoming

interface {
  price: number; // 12.12
  currency: string; // "USD"
  date: Date; // 1231232131
}

was too big a drop in readability for me. There were a bunch of other examples too. Maybe I'll decide it's worth it in the end, but leaving for now.

Busy working on the linting rules but it will take a while and is not a top priority, but we'll get there eventually :) Obviously a subset of prettiers features can be done with eslint -fix. Let's see how this develops, I'll continue to update on anything significant.

pudgereyem commented 3 years ago

@gadicc, thanks for circling back on this.

Regarding Prettier

I understand where you're coming from. But I'll try to make an argument for why Prettier is indeed worth it. Auto formatting with Prettier will not only make the formatting more consistent, but also make our lives so much easier when more contributors join.

Regarding your example with the inline comments, I actually think Prettier's way of doing makes more sense. I did like indented inline comments in the past, but the bad thing about it is that you'll have to change lines of code that hasn't actually changed if the length of one property changes. See example below.

interface {
  price: number;     // 12.12
  currency: string;  // "USD"
  date: Date;        // 1231232131
}

What happens if we extend this interface to have a new property internationalMarketExchange (just as an example)?

interface {
  price: number;     // 12.12
  currency: string;  // "USD"
  date: Date;        // 1231232131
  internationalMarketExchange: string; // "NASDAQ"
}

We would then need to change the indentation of the three lines above it as well. This is not a good practice in my opinion, and the reason why I like Prettier's way of doing it better.

gadicc commented 3 years ago

Willing to give this a shot and see how it goes.

pudgereyem commented 3 years ago

@gadicc sweet, I'll try it out.

gadicc commented 3 years ago

Closing since I think this is all working now, please anyone let me know if anything seems weird.

pudgereyem commented 3 years ago

@gadicc awesome work man 🎉

I haven't worked on the project since the latest changes, but will let you know if I find anything.

gadicc commented 3 years ago

Thanks, @pudgereyem! :D