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

chore: migrate @nuxt/eslint-config from v0.1 to v0.3 #26

Closed

Conversation

shinGangan
Copy link
Contributor

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

migrating @nuxt/eslint-config from v0.1.1 to v0.3.0.
In addition, fix lint error.

Copy link

netlify bot commented Jul 5, 2024

βœ… Deploy Preview for nuxt-leaflet ready!

Name Link
πŸ”¨ Latest commit a6532ad
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt-leaflet/deploys/66878984ceaeca00085820c3
😎 Deploy Preview https://deploy-preview-26--nuxt-leaflet.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@shinGangan
Copy link
Contributor Author

Hello @Gugustinette , please review when you have a time πŸ™

.override('nuxt/typescript/rules', {
rules: {
// TODO: Discuss if we want to enable this
'@typescript-eslint/no-explicit-any': 'off',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The typing around the Leaflet ecosystem isn't the best at the moment, which result in such things being somehow required :

declare module 'leaflet.markercluster' {
  export const MarkerClusterGroup: any;
}
import { ref } from 'vue';
const map = ref(null) as any; // So we can call => map.value.leafletObject

So I agree with turning that rule off for now.
Maybe we should comment the line with something like "Required while typing around Leaflet's ecosystem isn't cleaned up." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the same way. In the end, it would be healthier to take it in the direction of removal.

So I agree with turning that rule off for now.

The scope of application of this ESLint rule is the developer of this module.
Therefore, I think there is a way to write it in README.md so that developers can understand it.

Maybe we should comment the line with something like "Required while typing around Leaflet's ecosystem isn't cleaned up." ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it make sense to add some sort of CONTRIBUTING.md file to make it clear + add basic contribution stuff ? (Something like https://github.com/nuxt-modules/i18n/blob/main/CONTRIBUTING.md I guess)

I prefer not to overload the README file if possible (I mean I hate it when I open a git repo and the Readme is 1000 lines lol)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which mean we could remove the "Development" section of the README.md and make a link to CONTRIBUTING.md at the top of the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the contribution guide would be better since it's for development.
Since the content is different from this PR, it would like to create a separate issue and create a contribution guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, a contribution guide was recently added to Nuxt Script. This may be helpful.

nuxt/scripts#109
https://scripts.nuxt.com/docs/getting-started/contributing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you can create a separate issue ! πŸ˜„
The contributing guide could be in the "About" section of the doc, should look better.

For this PR just keep a comment on the file and I'll merge.

Copy link
Collaborator

@Gugustinette Gugustinette Jul 27, 2024

Choose a reason for hiding this comment

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

@shinGangan Can you update the comment with "Required while Leaflet's typing isn't improved" ?
(Also, maybe it's worth pulling last changes and run the lint command again to make sure any new files are linted correctly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gugustinette

Sorry, I overlooked it...

I'll respond to added the comments!
Also, there's a conflict, so I'll address that as well.

@Gugustinette
Copy link
Collaborator

I handled everything here : #36

So this is no longer necessary, but I used your config file thanks πŸ‘

@shinGangan
Copy link
Contributor Author

I was busy overwhelmed, so thank you for taking care of it! πŸ™

@Gugustinette

@shinGangan shinGangan deleted the chore/migrate-nuxt-eslint branch August 5, 2024 11:14
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.

2 participants