[plugin] relay project set up by pants `idea-plugin` goal.

Review Request #3670 — Created April 11, 2016 and submitted

wisechengyi
intellij-pants-plugin
from_cli
124
3664
pants-reviews
benjyw, nhoward_tw, peiyu, stuhood, zundel

This is intellij plugin side of work to handle Pants' idea-plugin goal https://rbcommons.com/s/twitter/r/3664/.

Also includes the changes:
1. remove target names from all settings, used relative target specs instead.
2. checks for idea-goal version and notify user if version is out of range.

https://travis-ci.org/pantsbuild/intellij-pants-plugin/builds/138480876

  • manually tested with pants/examples/ and twitter intenals.
  • 0
  • 0
  • 14
  • 5
  • 19
Description From Last Updated
WI
WI
ST
  1. Can you update the review description to explain or, better yet, point to a doc blurb in the code explaining what the strategy is here?

    1. added doc for convertToPantsProject

  2. 
      
SU
  1. 
      
  2. common/com/twitter/intellij/pants/model/PantsExecutionOptions.java (Diff revision 3)
     
     
     
     
     
     
     
     

    Add Javadoc comments explaining what these are.

  3. Add javadoc. What does it mean for something to be a potential Pants project? (Are all Pants projects guaranteed to be potential Pants projects? Do you expect to frequently have false positives, or only in unlikely edge cases?) How is it determined that something is a potential Pants project?

    1. renamed to isSeedPantsProject. a seeded pants project means it has the properties created by pants idea-plugin goal.

  4. This seems like a pretty important method. It deserves a Javadoc comment explaining what it does, its side effects, performance characteristics, and when it's appropriate to call.

    1. At the point I am not able to give an accurate doc on this one yet.

  5. What does this test mean? Is a potential Pants project something that is not a Pants project but could become one in the future?

    1. as discussed offline, it is a seeded pants project with the properties created by pants idea-plugin goal. also renamed to reflect so.

  6. Why does the constant lack an "_ONLY" when the command line ends in "-only"?
    1. This is one of the TODOs to clean up some options and constant. https://github.com/pantsbuild/intellij-pants-plugin/issues/114

  7. I'm not familiar with the style guide you're following, but I would either alphabetize all imports in one block or put the standard language imports in a separate block at the top (possibly followed by another block for libraries and finally a block specific to this application).

    1. I believe it is alphabetical for non standard block, then a block for the standard. Basically following the codestyle set in the repo, and it handles the import optimization.

  8. Javadoc is missing. What does this do?

  9. ** isn't significant inside the body of a function. Just use multiple lines of // comments.
    1. as discussed offline, these help intellij reference the links

  10. It's not clear how you could possibly convert a non-Pants project to a Pants project. I think you must mean something more specific.

    1. same as seed project conversion, as mentioned above.

  11. I'd swap these 3 lines with lines 87 & 88 above (keeping the blank line between these two sections). These go more with the definition of targetAddresses, while the definitions of manager and settings go with the call to setLinkedProjectSettings() below.

  12. What side effects does this have? Is this really the right place to have an effect on the UI?

    1. There is no side effect. The issue here is that the project started out as an empty one, thus not pants project, so this is the make sure the GUI parts is set correctly after the project is converted to a Pants project.

    1. Insert space after comma.
    2. What does this do? Why do you do it?
    1. corrected. and moved to PantsProjectComponentImpl.java. for the same reason above.

  13. Is "else" on a separate line from "}" the standard within this project? I'm more familiar with putting them on the same line.

    1. just following the default codestyle.xml in the repo

  14. Why do these two cases work differently?

    1. Initially the targets are obtained via project_path:targetNames. With project launched from CLI, we know that info already and don't have to that calculation.

  15. Why are these fields not final?

  16. Why is this separated from the above? The order of fields and the order of constructor arguments should match. You should also have field javadocs explaining what these fields mean.

    1. corrected and comment added below with the constructor. thanks

  17. Add javadoc defining what these arguments mean.

  18. src/com/twitter/intellij/pants/settings/PantsExecutionSettings.java (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    Do you really want the lists returned to be mutable?

  19. 
      
WI
WI
SU
  1. Sorry, I forgot to submit this comment the other day while we were talking. Maybe you've already done this by now or found a good reason not to.

  2. src/com/twitter/intellij/pants/components/impl/PantsProjectComponentImpl.java (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     

    Consider making the data model stuff above one function, and this GUI manipulation another function, and have a higher-level function that calls both of these in sequence.

    1. extracted to a private method.

  3. 
      
WI
WI
PE
  1. 
      
  2. have a factory method createDefault, replace false, true, false with static variable with meaningful names

  3. looks like these two are mutually exclusive so we onlyy need one, once the settings are converted internally here there should be no difference.

    1. combined them and using targetSpecs instead.

  4. also add comment why this is not immutable, unlike PantsExecutionSettings

    1. reverted to mutable. otherwise 2nd time the project opens, IJ is unable to retrieve the settings somehow, resulting in errors.

  5. feels strange to have read-only read but setter has no check at all.

    1. reverted, so still mutable.

  6. 
      
WI
PE
  1. 
      
  2. why do we need to to restrict both max and min? only curious since other functions requires just the min version

    1. This aims to prepares for any breakage we might introduce from pants side, in which case we can adjust the version to be greater than 0.1.0 so the plugin will know. I will make this better documented.

  3. depending on the lifecycle of the seedPantsProject this may not need to be a public interface, i.e, only valid prior to conversion

  4. make pants_idea_plugin_version a constant

    1. added inner class with constants

        public static boolean isSeedPantsProject(@NotNull Project project) {
          class SeedPantsProjectKeys {
            private static final String PANTS_IDEA_PLUGIN_VERSION = "pants_idea_plugin_version";
          } 
          ...
        }
      
  5. Q: once 'seedPantsProject' are converted, they will be pants projects right? and they are done at the initial loading time before refresh?

    1. seed project is converted by this refreshAllProjects function, so it has be checked here.

      I hope this answer to the comment above as well.
      "depending on the lifecycle of the seedPantsProject this may not need to be a public interface, i.e, only valid prior to conversion"

    2. that's how it's implemented by this review... but logically the conversion happens only once in projectOpened, while PantsUtil.refreshAllProjects may be called multiple times.

      if so, seems to me we can move the public method to where is close to projectOpened.

    3. Decided to leave isSeedPantsProject in PantsUtil becase:
      1. isPantsProject is in PantsUtil, and isSeedPantsProject does something similar in nature.
      2. Moving isSeedPantsProject closer to projectOpened and adding forceConvert to PantsUtil.refreshAllProjects may introduce some confusion on whether forceConvert should happen.

      Nonetheless, always open to refactoring if there's a better workflow discovered down the road.

  6. this seems implementation detail already explained in PantsUtil.isSeedPantsProject can remove from here

  7. 
      
WI
PE
  1. Ship It!
  2. 
      
WI
Review request changed

Status: Closed (submitted)

Change Summary:

f3566aba3022844632aa0f0a7ca5ed9cfb8aac4e thanks!

Loading...