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

adding linting for typescript #43

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

Conversation

bdomen-ggl
Copy link

Adding GTS as a linter for typescript.
GTS wraps eslint, eslint for react, and prettier and comes with default (yet overridable) configurations.
This is to support the React/Typescript UI work in cross-media-measurement.

@wfa-reviewable
Copy link

This change is Reviewable

@bdomen-ggl bdomen-ggl force-pushed the bdomen-typescript-linting branch from 2f117da to 91d49fa Compare July 18, 2023 15:36
@bdomen-ggl bdomen-ggl requested review from SanjayVas and removed request for SanjayVas July 18, 2023 15:48
@@ -50,6 +50,10 @@ runs:
shell: bash
run: pip3 install --user cpplint

- name: Install gts
Copy link
Author

Choose a reason for hiding this comment

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

Where should I setup GTS? I think this is the wrong spot despite cpplint installation is right above. setup-linters seems better, but everything is handled in the index file. I could just add this as another step in setup-linters if it makes sense to move it there.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bdomen-ggl)


lint/action.yml line 53 at r1 (raw file):

Previously, bdomen-ggl wrote…

Where should I setup GTS? I think this is the wrong spot despite cpplint installation is right above. setup-linters seems better, but everything is handled in the index file. I could just add this as another step in setup-linters if it makes sense to move it there.

The way it's split currently is that setup-linters is for setting up linters that are downloaded/installed manually. Those that are installed using a package manager are installed here, so this is the right place.


lint/action.yml line 55 at r1 (raw file):

    - name: Install gts
      shell: bash
      run: npm install --save-dev gts eslint eslint-config-react-app prettier

Can you explain why we use this flag? Apologies, I don't know npm that well.

Code quote:

--save-dev

Copy link
Author

@bdomen-ggl bdomen-ggl left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


lint/action.yml line 55 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Can you explain why we use this flag? Apologies, I don't know npm that well.

Super specifically, --save-dev will install packages to a dev-dependency section. These dependencies will not be installed when someone else installs your package like npm install MyPackage. If you clone the repo and run npm install it will install all packages (both dependencies and dev dependencies).

--save-dev is for dependencies that are strictly for the developers of the package, usually for things like test frameworks, bundling, linting, etc. Without the dependency the app will still work fine.

For example, things like react and react-router are necessary for an app to work (react as the UI framework and react-router to navigate pages within the SPA). jest would be a dev dependency since it isn't used by the app. This helps npm by not installing packages that an end user wouldn't need.

In this particular case, there's not really an "app" in the traditional sense, so it doesn't really matter if we have it as dev dependencies or regular dependencies. I left it out of habit since linting is generally installed in this way.

@bdomen-ggl bdomen-ggl marked this pull request as ready for review July 21, 2023 14:25
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.

3 participants