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

GitHub Actions: Implement ESlint #1442

Open
17 tasks
Tracked by #2651
akibrhast opened this issue Apr 23, 2021 · 31 comments · May be fixed by #5024
Open
17 tasks
Tracked by #2651

GitHub Actions: Implement ESlint #1442

akibrhast opened this issue Apr 23, 2021 · 31 comments · May be fixed by #5024
Labels
Added to dev/pm agenda Complexity: Large Dependency An issue is blocking the completion or starting of another issue Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly GHA New Project Board compatible This GitHub Action issue does not reference columns and will work with the new board role: back end/devOps Tasks for back-end developers size: 2pt Can be done in 7-12 hours

Comments

@akibrhast
Copy link
Member

akibrhast commented Apr 23, 2021

Dependency

#1441

Overview

We need to implement a linter for our javascript code to standardize and maintain the code properly.

Action Items

  • Create a list of possible bare minimum esLint settings that we can turn on
    • During one of the developer meetings, discuss the possible settings with the team and finalize a list of settings to implement
    • You may reference and use the list from this comment
  • Implement a GitHub action that triggers on pull request or on push to the gh-pages branch
    • The action should lint javascript files. The recommended tool for linting is Super Linter
    • Create the action as part of your local fork of the repository
    • Run the action on js files that should end in a success, and ensure that the action actually succeeds
    • Run the action on js files that should end in a failure, and ensure that the action fails properly based on the correct settings
    • Demo your prototype to the team and/or the technical lead to get approval
    • After approval, create a PR for review
    • Ensure that the linter does not trigger or break on liquid syntax
  • For merge team: Release dependency on Create a wiki page for linters used by the HfLA website team #3230

Resources/Instructions

Additional resources:

@qiqicodes

This comment has been minimized.

@qiqicodes qiqicodes added the To Update ! No update has been provided label May 9, 2021
@AmyMendelsohn

This comment has been minimized.

@AmyMendelsohn AmyMendelsohn removed their assignment May 10, 2021
@averdin2 averdin2 self-assigned this May 12, 2021
@averdin2
Copy link
Member

averdin2 commented May 12, 2021

@averdin2 averdin2 removed the To Update ! No update has been provided label May 12, 2021
@qiqicodes
Copy link
Member

@averdin2 Can you let us know your progress in regards to this issue? Thank you =]

Progress:
Blockers:

@qiqicodes qiqicodes added the To Update ! No update has been provided label May 15, 2021
@averdin2
Copy link
Member

averdin2 commented May 18, 2021

@wendywilhelm10

This comment has been minimized.

@Sihemgourou

This comment has been minimized.

@Sihemgourou

This comment has been minimized.

@aliibsin

This comment has been minimized.

@sayalikotkar sayalikotkar added the Feature Missing This label means that the issue needs to be linked to a precise feature label. label Jun 20, 2021
@Sihemgourou Sihemgourou added Feature: Standards and removed Feature Missing This label means that the issue needs to be linked to a precise feature label. labels Jun 20, 2021
@Sihemgourou

This comment has been minimized.

@Sihemgourou Sihemgourou removed the To Update ! No update has been provided label Jun 27, 2021
@averdin2 averdin2 removed their assignment Jun 27, 2021
@Sihemgourou Sihemgourou added the To Update ! No update has been provided label Jun 28, 2021
@github-actions

This comment has been minimized.

@Sihemgourou
Copy link
Contributor

@averdin2 @sanchece do you have some news about this issue ?

@SAUMILDHANKAR SAUMILDHANKAR added the size: 2pt Can be done in 7-12 hours label Mar 8, 2022
@ExperimentsInHonesty ExperimentsInHonesty added the Dependency An issue is blocking the completion or starting of another issue label Mar 19, 2023
@ExperimentsInHonesty ExperimentsInHonesty removed the Dependency An issue is blocking the completion or starting of another issue label May 15, 2023
@ronaldpaek ronaldpaek self-assigned this Jul 13, 2023
@github-actions
Copy link

Hi @ronaldpaek, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:-
i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?)
ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

@ronaldpaek
Copy link
Member

i. Friday - Sunday
ii. Sunday, maybe; never implemented a linter before, but I really want to get this done cause I don't like seeing red squigglies on my screen.

@ronaldpaek
Copy link
Member

@macho-catt Hi, I'm looking at the previous SCSS linter rules and only selected a few sets of rules. Usually, I opt for the standard config rule set, which is usually recommended and will ensure more consistency. And I reviewed the earlier comments to figure out what rules to apply. I know I usually apply eslint:recommended, and know that will apply pretty good robust setup rules for my js files without enforcing any of the stricter styling rules like semicolons, training commas, etc... Should I set each rule individually and the least amount of rules applied? I considered applying eslint:recommended by itself or with eslint:standard.

I also have another concern/suggestion as well. I know some of the js files I've checked also contain liquid syntax in them, which is why I currently get so many lint warnings. I checked the other day and set the file to be recognized as a liquid file, and the errors went away, but then we lost the IntelliSense, so that's not helpful plus, we want the linter to catch errors. And I believe even if we get eslint to work for JS stuff since the liquid syntax is in the js files, I think it would still throw an error and complain, and we would have to tell es-lint to ignore this block of code manually, like the example shown below.

/* eslint-disable */
{% assign name = 'world' %}
/* eslint-enable */

console.log('Hello, ' + name + '!');

I think this solution would be okay for the first step, and then, we can separate the liquid portion, the data, and the js portion, the logic.
potential issues

🙇
@roslynwythe @t-will-gillis

@ronaldpaek
Copy link
Member

Hello, I did some research and did a lot of PRs to my repo. I'm not saying it's perfect, but I was just doing some testing, and I first got the super-linter to work for the JS files, then after researching, I saw we can use super linter/slim version which is a little smaller so the GHA/linting finishes faster.

CleanShot 2023-07-15 at 19 13 37@2x CleanShot 2023-07-15 at 19 13 21@2x CleanShot 2023-07-15 at 19 05 43@2x CleanShot 2023-07-15 at 19 06 09@2x

@ronaldpaek
Copy link
Member

I then figured out and tested the pre-commit hook feature others mentioned earlier, basically having the linter enforce rule check before making a pull request, which would ensure that every review is formatted correctly and no immediate glaring issues.

It took a bit of figuring out and testing, but I was able to get do pre-commits with JS and CSS

CleanShot 2023-07-15 at 19 42 13@2x CleanShot 2023-07-15 at 19 42 05@2x CleanShot 2023-07-15 at 19 45 45@2x

So the pre-commit works, but you can bypass that if you want and commit anyway. Since we already have our linter set up, it isn't a problem. It runs like normal, as if we never had a pre-commit linter going on.

CleanShot 2023-07-15 at 19 49 25@2x CleanShot 2023-07-15 at 19 49 20@2x

@ronaldpaek
Copy link
Member

So I realize all these photos show prettier installed and, yes, prettier, was the last thing I was testing, and they play well with each other. I left just the prettier default setting because I know someone was ticking off every rule set if we know what rules to apply, I think it should be very fast to apply. but seeing as I was committing and pushing/saving files, I don't think the way I have it set up it will try to prettify the whole codebase.

I think some of the rules can be adjusted for prettier as we need. I think we can also adjust it to warn but still be able to make commits as well. Like this head tag thing for liquid/front matter, it doesn't know that that's not an HTML page.

CleanShot 2023-07-15 at 19 57 12@2x

I think this is useful; we can quickly see where inconsistencies are, and should be easy to fix.

CleanShot 2023-07-15 at 19 57 12@2x

I think if a codebase had single and double quotes switching back and forth, it could potentially stress new team members. I know it would stress me out, haha.

CleanShot 2023-07-15 at 20 03 12@2x

And in this example, I clicked a random js file, which already gives me a useful measure. About a variable that isn't defined, I think those things are super helpful on those big files I tend to get overwhelmed. 😆

@ronaldpaek
Copy link
Member

Lastly, my last task was to fix the liquid syntax, and actually, that was kind of the reason why I took on this task, but I guess all of this stuff is tied together. I'm not that detailed and organized, so I took a lot of videos and pictures and am reporting my findings.

CleanShot 2023-07-15 at 20 05 33@2x

I could not solve this issue yet, but I am thinking of reorganizing the liquid data, probably in a separate file. I'm just not 100% confident about how the data flows and is used, yet I still need to review the codebase some more.

CleanShot 2023-07-15 at 20 09 00@2x

Oh yes, I actually ended up creating a lot of config files. I wanted to mention I had to init the root folder to node so I could install things on the package.json.

I know I don't think I didn't it when I just did the linter for JS, but when I was looking into the pre-commit stuff since it's a front-end/client side thing, you need a package.json to be in the root, but I'm just reporting my findings I'm sure there is a better way, but at least we have some data, that this is all possible, and it can be done incrementally without breaking everything.

CleanShot 2023-07-15 at 20 13 04@2x CleanShot 2023-07-15 at 20 13 23@2x

Okay, I am done with my report. 😄
@roslynwythe @t-will-gillis @blulady @ExperimentsInHonesty

@ronaldpaek
Copy link
Member

I hope not to sound like a salesman for linting and formatting software. But now I'll post more photos to convince us why we want this.

CleanShot 2023-07-15 at 21 39 33@2x CleanShot 2023-07-15 at 21 40 22@2x

I think this is a good thing. Large codebases are intimidating, and it's worse if much of the code is not even. Being used, and if it is harder for new people to adopt the style we want, it will ultimately take longer to ramp up.

CleanShot 2023-07-15 at 21 43 41@2x

Clean Code = Happy Coder. It's like studying on a cluttered desk. 😆

CleanShot 2023-07-15 at 21 45 12@2x

I'd be willing to do all transferring/updating and 10000 commits to update every 1 week if you want ;)

CleanShot 2023-07-15 at 21 47 12@2x

@ExperimentsInHonesty
Copy link
Member

@ronaldpaek why did you unassign? Where should this issue be moved?

@djbradleyii djbradleyii added the ready for dev lead Issues that tech leads or merge team members need to follow up on label Jan 9, 2024
@ExperimentsInHonesty ExperimentsInHonesty removed the ready for dev lead Issues that tech leads or merge team members need to follow up on label May 7, 2024
@ExperimentsInHonesty
Copy link
Member

Once this issue comes out of the icebox, This issue will need a really good read, and then a decision on what to do next by the dev lead.

@ExperimentsInHonesty ExperimentsInHonesty added the Dependency An issue is blocking the completion or starting of another issue label May 7, 2024
@ExperimentsInHonesty
Copy link
Member

  • Figure out which js files use liquid (and the least liquid in any file)
  • Refactor a js file that uses liquid into two separate files
  • Roll out plan for implementation

@ExperimentsInHonesty ExperimentsInHonesty added the GHA New Project Board compatible This GitHub Action issue does not reference columns and will work with the new board label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added to dev/pm agenda Complexity: Large Dependency An issue is blocking the completion or starting of another issue Feature: Board/GitHub Maintenance Project board maintenance that we have to do repeatedly GHA New Project Board compatible This GitHub Action issue does not reference columns and will work with the new board role: back end/devOps Tasks for back-end developers size: 2pt Can be done in 7-12 hours
Projects
Development

Successfully merging a pull request may close this issue.