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

feat(pack): java. enable niceities with jdtls and downloading source … #524

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

jay-babu
Copy link
Contributor

…code from libraries

📑 Description

enable niceities with jdtls and downloading source code from libraries

ℹ Additional Information

@github-actions
Copy link

github-actions bot commented Aug 17, 2023

Review Checklist

Does this PR follow the [Contribution Guidelines](development guidelines)? Following is a partial checklist:

Proper conventional commit scoping:

  • If you are adding a new plugin, the scope would be the name of the category it is being added into. ex. feat(utility): added noice.nvim plugin

  • If you are modifying a pre-existing plugin or pack, the scope would be the name of the plugin folder. ex. fix(noice-nvim): fix LSP handler error

  • Pull request title has the appropriate conventional commit type and scope where the scope is the name of the pre-existing directory in the project as described above

  • README is properly formatted and uses fenced in links with <url> unless they are inside a [title](url)

  • Proper usage of opts table rather than setting things up with the config function.

@Uzaaft
Copy link
Member

Uzaaft commented Aug 20, 2023

@AstroNvim/astrocommunity-maintainers Anyone of you who uses Java, and thinks this should be added into the pack?
Imo it seems like something that should be enabled by the user, not by the pack.

@owittek
Copy link
Member

owittek commented Aug 20, 2023

Didn't we have something similar with the rust lsp where we were planning to just add it to the readme? But if you ask me we can just merge it and if anyone complains it can still be removed

@Uzaaft
Copy link
Member

Uzaaft commented Aug 20, 2023

Didn't we have something similar with the rust lsp where we were planning to just add it to the readme? But if you ask me we can just merge it and if anyone complains it can still be removed

We added that actually, lsp-settings.
Up to you hehe, if you want to go ahead and merge this one

@jay-babu
Copy link
Contributor Author

@owittek @Uzaaft what will it take to get this merged?

@Uzaaft
Copy link
Member

Uzaaft commented Aug 26, 2023

My view is that these changes might be considered as too opinionated for someone who uses Java, so I'm just letting it stay until one of the more java savy maintainers take a look.

@bubuntux
Copy link

bubuntux commented Sep 3, 2023

the star import and static imports seems very opinionated i do agree with it, but i know of a lot of projects/companies that do not agree with it, i would argue that would be better leave it off the pack

@jay-babu
Copy link
Contributor Author

jay-babu commented Sep 3, 2023

the star import and static imports seems very opinionated i do agree with it, but i know of a lot of projects/companies that do not agree with it, i would argue that would be better leave it off the pack

Instead of worrying about what other projects/companies would like, can we focus on the astrocommunity and what they want?

PS, I've worked at fairly large companies and they have this rule. I am okay with removing what seems too opinionated, but I ask that this is inline with the astrocommunity wants not what other projects would do. Most of the PR is just enabling features that provide more information. there isn't much else happening. The star imports just ensures LOD, so it shouldn't be an issue

@Uzaaft
Copy link
Member

Uzaaft commented Sep 4, 2023

the star import and static imports seems very opinionated i do agree with it, but i know of a lot of projects/companies that do not agree with it, i would argue that would be better leave it off the pack

Instead of worrying about what other projects/companies would like, can we focus on the astrocommunity and what they want?

PS, I've worked at fairly large companies and they have this rule. I am okay with removing what seems too opinionated, but I ask that this is inline with the astrocommunity wants not what other projects would do. Most of the PR is just enabling features that provide more information. there isn't much else happening. The star imports just ensures LOD, so it shouldn't be an issue

Astrocommunity tries to provide a sane and sensible default. Not sure if this qualifies as that, but I'll let another Java using maintainer be the judge of that.

@owittek
Copy link
Member

owittek commented Sep 4, 2023

Personally I'm fine with it, people can always check the sources if something seems wrong for them and PR changes or override it in their config

@sjcobb2022
Copy link
Contributor

As the person who made this pack, this seems fine if everyone is alright with it. Definitely seem like some sane defaults.

I mean it's all overrideable anyway, as the user can configure it however they like through the opts :)).

@owittek
Copy link
Member

owittek commented Sep 9, 2023

I'll just go ahead and merge this then, I see the benefit of having sane defaults and there's always the possibility to dispute this decision

Copy link
Member

@owittek owittek left a comment

Choose a reason for hiding this comment

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

LGTM!

@owittek owittek merged commit 8284d88 into AstroNvim:main Sep 9, 2023
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.

5 participants