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 icon, issue navigation in Intellij IDEA and Toolbox #1206

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Mar 18, 2024

Effect

ICON

截屏2024-03-18 19 18 13 截屏2024-03-18 19 18 27

Issue navigation

截屏2024-03-18 19 18 22

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Cool!

@He-Pin He-Pin added this to the 1.1.0-M1 milestone Mar 18, 2024
@pjfanning
Copy link
Contributor

Why do we need this? ASF is vendor neutral. I think we need a mailing list discussion. Please don't merge this until we get 1.1.0-M1 released.

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

I'd prefer more discussion. I don't want the discussion to delay 1.1.0-M1.

I'm travelling today. If this calls non ASF links without user consent then it is banned by ASF rules. I will need to check if this compliant.

@Roiocam
Copy link
Member Author

Roiocam commented Mar 18, 2024

If this calls non ASF links without user consent then it is banned by ASF rules.

I can't quite understand this. This PR is just a pre-set for developers in IntelliJ IDEA. I have seen the Apache flink already provide this, so maybe this will not be banned by ASF rules.

Anyway, I'm open about whether to provide presets. In some ways, it can improve the developer experience, but on the other hand, it requires some small maintenance costs.

@pjfanning pjfanning modified the milestones: 1.1.0-M1, 1.1.0-M2 Mar 18, 2024
@pjfanning
Copy link
Contributor

I haven't yet had time to test this. I would like to know why this is not in https://github.com/apache/incubator-pekko-sbt-paradox - why would we want to copy this javascript into every single Pekko repo instead of putting the code in a central place that can be reused in different repos?

I still think this should be discussed in an dev@pekko.apache.org mail thread before we invest more time in it.

I don't see why the 'Edit this Page' functionality on pages like https://pekko.apache.org/docs/pekko/current/typed/actor-lifecycle.html is not enough.

@pjfanning
Copy link
Contributor

pjfanning commented Mar 18, 2024

Ok - I think I understand this now. The description has zero description and is very confusing. I thought this was about updating the web site to add a button to support IntelliJ.

It seems that this is actually instead about adding IntelliJ config files. You know that the links that are in our md files actually have the full URLs.

Links look like this in our .md files.

[Link Name](https://host.name/path/to)

Example:

([PR903](https://github.com/apache/incubator-pekko/pull/903))

Why can't IntelliJ just use the links in the md files? Why do we need to check-in config files with regexs to form URLs? -- the URLs are already in the .md file.

If IntelliJ can't handle this - shouldn't we report a bug to IntelliJ? Why check in complicated workarounds in our repos and have to maintain that -- when this looks like a shortcoming of IntelliJ.

Many of us using Metals and Visual Studio. We are not looking to check in metals or Visual Studio config into each of our 20 or so Git repos.

@Roiocam
Copy link
Member Author

Roiocam commented Mar 19, 2024

Why can't IntelliJ just use the links in the md files? Why do we need to check-in config files with regexs to form URLs? -- the URLs are already in the .md file.

This PR provides two presets:

  • display project icon
  • generate issue navagation(link) on version control UI.

It doesn't relate to markdown files. If the contributor wants to check the context of some of the commits, through this PR, he can jump directly by clicking the ISSUE number on VCS.

VSCode already provides this feature via the git lens extension.

But there is also an issue with this PR, that is, it can not distinguish the ISSUE number from Akka committed before,
and the VSCode can not either.

IntelliJ IDEA settings

截屏2024-03-19 09 43 49 截屏2024-03-19 09 44 01

VSCode extension

截屏2024-03-19 09 54 09

@mdedetrich
Copy link
Contributor

In principle I am also against putting IDE specific functionality into the main repo's. Pekko should be entirely IDE/editor agnostic and if we go down this road then we end up polluting the codebase with all of these bespoke editor/IDE workarounds.

We have had these kind of PRs in the past with my response being the same, and although it might be a bit drastic/crazy I think the best solution to this would be a separate Intellij plugin which adds all of these bells and whistles for Pekko projects. Note that I am not alone in this, i.e. for the ZIO ecosystem there is actually an Intellij ZIO plugin which also adds a lot of niceties for example better error/compilation hints and inspections/refactorings, we can do the same with Pekko (including solving this issue)

@raboof
Copy link
Member

raboof commented Mar 19, 2024

In principle I am also against putting IDE specific functionality into the main repo's. Pekko should be entirely IDE/editor agnostic and if we go down this road then we end up polluting the codebase with all of these bespoke editor/IDE workarounds.

I don't feel so strongly about this: while on the one hand adding this to every pekko repo adds some noise, if it improves the contributor experience (esp. for casual/new contributors) it might be worth it.

for the ZIO ecosystem there is actually an Intellij ZIO plugin which also adds a lot of niceties for example better error/compilation hints and inspections/refactorings, we can do the same with Pekko (including solving this issue)

It seems this is a plugin for working with ZIO rather than working on ZIO.

@mdedetrich
Copy link
Contributor

It seems this is a plugin for working with ZIO rather than working on ZIO.

I was more illustrating that the idiomatic way to solve this issue is with separate plugins (and afaik Intellij Plugins can pretty much do anything) even if its just for Pekko developers (and such a hypothetical plugin can do both anyways)

In any case there appears to be issues with the PR, there also may be existing Intellij plugins (i.e. a github one?) which we can document that solves this more comprehensively.

@laglangyue
Copy link
Contributor

I added this locally, which is indeed useful. But adding the idea folder to the project does indeed violate Apache style.
We can add .idea folder rule to the ignore file, then we can add a developer tool page in our website, which providing some suggestions from developers, including this setting and some plugins in idea.

Look at this pages, it is very useful.
https://spark.apache.org/developer-tools.html

@mdedetrich mdedetrich closed this Mar 20, 2024
@mdedetrich mdedetrich reopened this Mar 20, 2024
@He-Pin
Copy link
Member

He-Pin commented Mar 21, 2024

I think this is useful, even scala/scala has IDE-specified files.

@He-Pin
Copy link
Member

He-Pin commented Jul 16, 2024

There is a PR in Scala3 too, scala/scala3#21188, I think this would be nice .

.idea/vcs.xml Outdated Show resolved Hide resolved
.idea/vcs.xml Outdated Show resolved Hide resolved
Roiocam and others added 2 commits July 29, 2024 23:59
Co-authored-by: Arnout Engelen <arnout@engelen.eu>
Co-authored-by: Arnout Engelen <arnout@engelen.eu>
@pjfanning pjfanning dismissed their stale review August 7, 2024 16:58

old review

@pjfanning pjfanning modified the milestones: 1.1.0, 1.1.1 Aug 23, 2024
@pjfanning pjfanning modified the milestones: 1.1.1, 1.1.2 Sep 9, 2024
@pjfanning pjfanning modified the milestones: 1.1.2, 1.1.x Sep 27, 2024
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.

6 participants