medic / cht-interoperability

CHT - eCHIS interoperability project

GNU Affero General Public License v3.0

2 stars 3 forks source link

Fixed cht-config bug. #41

Closed samuelimoisili closed 1 year ago

samuelimoisili commented 1 year ago

Description

Turns out the entire bug was because of the parent ESLint script.

medic/interoperability#16

Code review checklist

[ ] Readable: Concise, well named, follows the style guide, documented if necessary.
[ ] Documented: Configuration and user documentation on cht-docs
[ ] Tested: Unit and/or e2e where appropriate
[ ] Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

andrablaj commented 1 year ago

@samuelimoisili I don't know enough about ESLint configuration to review this PR. Can you please share more details about what the problem was before and how your solution solves it? Thank you

samuelimoisili commented 1 year ago

@andrablaj When we run cht-conf, it tries to lint the code using the rules of the parent ESLint config. I ignored the child from the parent configuration, and I reset the child config to the base config we get when we initialise a new project because the codebase didn't fit the current ESLint config. If I was to lint the codebase with the previous configuration, the PR will be quite large and I wanted to make it smaller.

Should I apply the previous linting rules on the codebase?

andrablaj commented 1 year ago

Thank you for the clarifications @samuelimoisili! It was helpful in understanding what went wrong with the previous configuration.

Should I apply the previous linting rules on the codebase?

I will leave the decision to you, @lorerod and @njogz.

njogz commented 1 year ago

I think this is okay and I agree with your rationale for keeping this PR short. @garethbowen do you have a recommendation for a preferred eslint config to be used across all CHT products?

garethbowen commented 1 year ago

Why yes! We have a published eslint config here: https://github.com/medic/eslint-config

You should be able to npm install it and then depend on it and override/extend where appropriate.

njogz commented 1 year ago

Thanks @garethbowen. @samuelimoisili let's use this config.

njogz commented 1 year ago

I have created a new issue for the eslint config change. I will merge this for now so that it unblocks the other PRs.