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: Add support for specifying an IntelliJ module name prefix #349

Merged

Conversation

ryanhanks
Copy link
Contributor

Description

This change came about as a result of discussion around the 'melos_' string that is prepended to IntelliJ project module names, and the desire for that prefix to be blank.

The IntelliJ configuration parser currently assumes a single boolean value, so the parser has been updated to now support a map with which to specify intelliJ config.

The default value of enabled can now be overrode via an enabled key in the intellij map, and the parser continues support the current configuration approach, whereby the intellij key is set to a boolean value:

ide:
  intellij:
    enabled: false  # this is now the documented approach
ide:
  intellij: false        # the config parser will support this schema also

This map now allows for the specification of an alternative project module name prefix:

ide:
  intellij:
    moduleNamePrefix: ''   # set to empty string to override default value of 'melos_'

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 📝 docs -- Documentation

Notes

  • I added docs around the current integration with IntelliJ to the best of my knowledge. Let me know what I missed.

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2022

CLA assistant check
All committers have signed the CLA.

@ryanhanks ryanhanks changed the title Add support for specifying an IntelliJ module name prefix Feat: Add support for specifying an IntelliJ module name prefix Jul 10, 2022
@ryanhanks ryanhanks changed the title Feat: Add support for specifying an IntelliJ module name prefix feat: Add support for specifying an IntelliJ module name prefix Jul 10, 2022
@ryanhanks
Copy link
Contributor Author

ryanhanks commented Jul 10, 2022

Also, I didn't update the current melos.yaml config for this project, a I thought it might break CI. Lmk if we should include a change for that in this PR or if it can / should be addressed at another time.

@ryanhanks
Copy link
Contributor Author

Looks like I missed a couple things here. Updates to follow in the next day or few.

@blaugold
Copy link
Collaborator

Thanks for this contribution! Looks great, aside from the analyzer errors (just some style issue) and formatting.

Also, I didn't update the current melos.yaml config for this project, a I thought it might break CI. Lmk if we should include a change for that in this PR or if it can / should be addressed at another time.

I think it's fine to leave it as is for now.

@ryanhanks
Copy link
Contributor Author

Thanks blaugold 👍

For transparency, in addition to the style cleanup, I missed tracing the "melos_" prefix all the way down the stack, and it looks like it exists in hard-coded form beyond the areas I've addressed thus far, e.g.,

String pathPackageModuleIml(Package package) {
return joinAll([package.path, 'melos_${package.name}.iml']);
}
(I also noticed this value is hard-coded in the clean-up script)

@ryanhanks ryanhanks marked this pull request as draft July 11, 2022 12:00
@blaugold blaugold force-pushed the add_support_for_intellij_module_name_prefix branch from 05aa290 to 16bbc2a Compare August 15, 2022 07:09
@blaugold
Copy link
Collaborator

Hey @ryanhanks, I've rebased the PR and added the use of moduleNamePrefix in all places where module names are built.
For the run configurations, we should keep using the hard-coded prefix.
I think it's ready to be merged. WDYT?

@ryanhanks
Copy link
Contributor Author

Hey @blaugold

I pulled it down and gave it a run this morning, looks good to me!

I must say, on my repo of 26 projects, it was v nice to open IntelliJ and see the results of this work :)

Thanks for cleaning this up! Feel free to merge whenever you're ready.

@ryanhanks ryanhanks marked this pull request as ready for review August 19, 2022 22:39
@blaugold blaugold requested a review from Salakar August 23, 2022 22:58
Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

Nice!

@blaugold blaugold merged commit 1d2720f into invertase:main Aug 24, 2022
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.

None yet

4 participants