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

Add prefer-const linting rule to kolibri-tools #9867

Closed
rtibbles opened this issue Nov 23, 2022 · 23 comments · Fixed by #10130
Closed

Add prefer-const linting rule to kolibri-tools #9867

rtibbles opened this issue Nov 23, 2022 · 23 comments · Fixed by #10130
Assignees
Labels
DEV: frontend DEV: tools Internal tooling for development good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome

Comments

@rtibbles
Copy link
Member

Observed behavior

I quite frequently see uses of let in cases where the variable will never be reassigned. In this case const should be preferred for the variable declaration.

ESLint has a prefer-const rule https://eslint.org/docs/latest/rules/prefer-const that has some autofixing capability, so this could be added and any existing usages fixed.

Note it would be best to wait until #9698 has been merged to do this.

@rtibbles rtibbles added DEV: frontend help wanted Open source contributors welcome good first issue Self-contained, straightforward, low-complexity DEV: tools Internal tooling for development labels Nov 23, 2022
@marcellamaki marcellamaki added this to the 0.16.0 milestone Dec 2, 2022
@AbrahamOsmondE
Copy link

Hi! :)

I understand that this issue is still waiting for #9698 to be merged first, but would it be possible for me to take this up? I don't mind having to redo some stuff once the #9698 has been merged

@rtibbles
Copy link
Member Author

I think it may be possible to add this rule even without #9698 being merged, so please do go ahead and give this a go!

@rahulhumie
Copy link

i wanted to work on the issue please assign me the issue

@AbrahamOsmondE
Copy link

I think it may be possible to add this rule even without #9698 being merged, so please do go ahead and give this a go!

Thank you! Any advice on how I might be able to get the issue assigned?

@MAYANKpandey14
Copy link

Hey, is someone working on this issue ?

@rahulhumie
Copy link

rahulhumie commented Jan 20, 2023 via email

@MAYANKpandey14
Copy link

Should I wait for #9698 to get approved ?
One of the test it has failed is this .
Linting of JS, Vue, SCSS and CSS files...................................Failed

@MisRob
Copy link
Contributor

MisRob commented Jan 24, 2023

Thank you! Any advice on how I might be able to get the issue assigned?

@AbrahamOsmondE I think some of the core team members need to assign you. So I just did it as you were the first one to show interest. Please let us know in case you aren't working on it.

@MAYANKpandey14 @rahulhumie Thank you for your interest. As just mentioned, I assigned the issue to @AbrahamOsmondE for now. We are always grateful for contributions and if you'd like to work on Kolibri, there are many other issues open. It may be helpful to filter them by "help wanted", "good first issue", or "beginner friendly" tags.

@AbrahamOsmondE
Copy link

Thank you! Any advice on how I might be able to get the issue assigned?

@AbrahamOsmondE I think some of the core team members need to assign you. So I just did it as you were the first one to show interest. Please let us know in case you aren't working on it.

@MAYANKpandey14 @rahulhumie Thank you for your interest. As just mentioned, I assigned the issue to @AbrahamOsmondE for now. We are always grateful for contributions and if you'd like to work on Kolibri, there are many other issues open. It may be helpful to filter them by "help wanted", "good first issue", or "beginner friendly" tags.

Hi @MisRob , I'm currently trying to setup the development environment and I'm facing issues on the following section when trying to setup nodeenv on the virtual environment. I've tried looking for my error on google but can't seem to find it. May I know where might be a good place to ask?

@MisRob
Copy link
Contributor

MisRob commented Jan 24, 2023

@AbrahamOsmondE Thank you for asking, it'd be fine to post the error here and we'll do our best to figure it out with you

@AbrahamOsmondE
Copy link

@AbrahamOsmondE Thank you for asking, it'd be fine to post the error here and we'll do our best to figure it out with you

@MisRob I've been getting the following error when doing nodeenv -p --node=10.17.0 while having the kolibri pyenv up and running.
Screenshot 2023-01-25 at 9 38 31 AM

I used homebrew to install a node with version 16.x as mentioned in the document. Do let me know if any other detail is required from me. Thank you!

@MisRob
Copy link
Contributor

MisRob commented Jan 25, 2023

@AbrahamOsmondE, is this problem specific to Node v10 only? Are you able to proceed with the setup using Node v16?

@AbrahamOsmondE
Copy link

@AbrahamOsmondE, is this problem specific to Node v10 only? Are you able to proceed with the setup using Node v16?

@MisRob no the problem persists even when trying to use Node v16. Also tried using Node v19 and it didn't work too

@MisRob
Copy link
Contributor

MisRob commented Jan 27, 2023

I'm sorry to hear that. Unfortunately, I have no idea what's causing this problem, and I haven't heard yet from anyone who'd experienced this error.

The only idea I've gotten so far is that perhaps playing around with different virtual environment tools may be helpful? For example, I use venv and nvm and not nodeenv. I don't know if this could help, and I don't have Mac myself.

@MisRob
Copy link
Contributor

MisRob commented Jan 27, 2023

@AbrahamOsmondE ^

@MisRob
Copy link
Contributor

MisRob commented Jan 27, 2023

@AbrahamOsmondE Not related to that particular error, but if you get further in the setup process, one of my colleagues shared that instead of this step https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#checkout-release-v0-15-x (which also requires switching to Node v10 temporarily), it should be simpler to run kolibri manage provisiondevice. That way, we only need to make sure that Node v16 works.

@Gauravjadhav22
Copy link

i wanted to work on the issue please assign me the issue

@MisRob
Copy link
Contributor

MisRob commented Feb 15, 2023

Taking into account that we didn't hear back from @AbrahamOsmondE for some time, I'm removing the assignment. If someone wants to work on it, please let me know as soon as you start coding and I will make a new assignment.

@Akila-I
Copy link
Contributor

Akila-I commented Feb 16, 2023

Hi @MisRob , I have been in contact with Learning Equality for about last 2 weeks and I have successfully got setup Kolibri on my local computer. I land on this good-first-issue and I'd love to work on this if you could assign this issue to me.
Thanks in advance.

@MisRob
Copy link
Contributor

MisRob commented Feb 16, 2023

Hello @Akila-I, thank you for your interest. You're welcome to take this on.

@Akila-I
Copy link
Contributor

Akila-I commented Feb 18, 2023

Hi @MisRob , Thanks for assigning the issue to me.
I have imposed the prefer-const ESLint rule and made the necessary changes in the code.
Now running npm run lint-frontend exits without an error or warning.
Is there any other test do I need to run before make a PR.

@Akila-I
Copy link
Contributor

Akila-I commented Feb 18, 2023

@rtibbles , @MisRob I have made a PR #10130 . Maybe we can review there. Please do correct me if anything is not in the order.

@MisRob
Copy link
Contributor

MisRob commented Feb 22, 2023

Thank you for your contribution, @Akila-I. Yes, we will review changes in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend DEV: tools Internal tooling for development good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants