WINDUP-253 Refactor WindupProcessor/Impl #228

OndraZizka · 2014-09-01T18:00:03Z

Refactor WindupProcessor/Impl to simplify the API, move inner classes out and offload part of the code like prettyPrintRule() out of this important class.

WINDUP-272 Refactor GraphContextFactoryImpl and GraphContextImpl
WINDUP-253 Refactor WindupProcessor/Impl
WINDUP-254 Improve error message in RuleSubset.

lincolnthree · 2014-09-02T15:50:15Z

tests/core/src/test/java/org/jboss/windup/tests/application/WindupArchitectureTest.java

        RecordingWindupProgressMonitor progressMonitor = new RecordingWindupProgressMonitor();
-        processor.execute(progressMonitor);


Where is the configuration passed to the processor?

Ha! I wonder the tests passed without that :) Will fix.

lincolnthree · 2014-09-02T20:50:50Z

config/api/src/main/java/org/jboss/windup/config/loader/GraphConfigurationLoader.java

@@ -14,12 +14,12 @@
 public interface GraphConfigurationLoader
 {
    /**
-     * Loads all {@link Configuration}s from the system and returns the result.
+     * Loads all OCP Rewrite's {@link Configuration}s from the system and returns the result.


How about:

Loads all known {@link WindupRuleProvider} instances and returns the compiled {@link Rewrite} pipeline {@link Configuration}.

That's way better. I added "OCP Rewrite's" to make it clear what kind of configuration that is - because the term Configuration is a bit overloaded.

jsight · 2014-09-03T20:45:39Z

exec/api/src/main/java/org/jboss/windup/engine/WindupProcessorConfig.java

+
+    //<editor-fold defaultstate="collapsed" desc="get/set">
+
+    public Path getOutputDirectory()


Needs javadoc

OndraZizka · 2014-09-03T22:26:22Z

Currently there's a problem: In tests, GraphContext gets initialized before WindupProcessor boots. So the graph is already located in some default location. Once again: I think we need a proper bootstrap. More that in an email.

jsight · 2014-09-03T22:47:56Z

That only happens if you try to access the graph itself before setting the
graph directory.
On Sep 3, 2014 6:26 PM, "Ondrej Zizka" notifications@github.com wrote:

Currently there's a problem: In tests, GraphContext gets initialized
before WindupProcessor boots. So the graph is already located in some
default location. Once again: I think we need a proper bootstrap. More that
in an email.

—
Reply to this email directly or view it on GitHub
#228 (comment).

… out and offload part of the code like prettyPrintRule() out of this important class.

…art 1)

…ull/228 part 2)

needed by WINDUP-253 Refactor WindupProcessor/Impl

WINDUP-272 Refactor GraphContextFactoryImpl and GraphContextImpl - WIP WINDUP-253 Refactor WindupProcessor/Impl

…moved after development is done.

OndraZizka · 2014-09-11T01:03:44Z

Done, the tests pass. Rebasing after the dir structural change needs huge amout of work. I suggest rebasing that on top of this.

…NDUP-253 Merging the rebase.

OndraZizka · 2014-09-12T02:26:34Z

(22:17:01) Lincoln: ozizka: well, the code format (not a huge deal)
(22:17:18) ozizka-FN: Sure :) Sorry for that, I'll learn to find the spots.

(22:17:16) Lincoln: ozizka: there is also the strange relationship between WindupConfigurationModel and WindupConfig
(22:17:24) ozizka-FN: Right, I would like to fix that as next step in this path
(22:17:43) ozizka-FN: But this relation was there before too, only hidden.
(22:17:58) ozizka-FN: I only moved stuff to an object, now it's obvious
(22:17:58) ozizka-FN: I only moved stuff to an object, now it's obvious

(22:17:55) Lincoln: There are a few places where exceptions are created and logged just to display a trace, which is pretty confusing imo
(22:18:27) ozizka-FN: lincolnthree: It was most helpful. But I can get the stack otherwise.

(22:18:25) Lincoln: ozizka: the other issue for WINDUP-254 was included in this PR as well, and does not follow the solution we discussed afaik
(22:18:25) jbossbot: jira [WINDUP-254] Improve error message in RuleSubset.
(22:19:18) ozizka-FN: Right, without it I would not see what's happening. I can change it to log, although I don't understand why it's better.

(22:19:09) Lincoln: ozizka: and I want to look deeper at the GraphContextImpl just to maybe make some tweaks because the relationship with that and the config seem a bit strange
Lincoln lincolnthree
(22:19:47) Lincoln: ozizka: once you pass in a config, you can never figure out what config a graphContext was configured with (without getting the WCModel)
(22:20:47) ozizka-FN: lincolnthree: I agree - but as I said, that has not changed, only "objectified"
(22:21:54) Lincoln: ozizka: well, i re-added the getGraphDir method on GraphContext
(22:22:07) Lincoln: that had been removed but was used in some more recent tests. Not sure what to do with that yet, its an odd method
(22:23:06) ozizka-FN: It belongs to GraphContextConfig. Can ask that stored config. When it's stored :)
(22:23:35) Lincoln: ozizka: yeah that's what I did

I'd suggest to merge to stick with the PR state, if you consider it a bit improvement. And I'll address this next (or later).

jsight · 2014-09-12T02:56:09Z

exec/api/src/main/java/org/jboss/windup/engine/WindupProcessorConfig.java

+
+    //<editor-fold defaultstate="collapsed" desc="get/set">
+
+    public GraphLifecycleListener getGraphListener()


Needs javadoc

…nd a log message. GCI - don't use confusing new Exception() to get the stack trace. WindupProcessor - code format.

lincolnthree · 2014-09-12T11:07:08Z

Following up on Ondra's email to windup-dev.

Regarding the passing of variables to WindupConfigurationModel, I'd really love to see us choose one method of configuration over the other. This is perhaps a non-trivial problem because of our use of implicit initialization in many test-cases, but in the end it should be doable (and should be done, IMO) I'd like to look into how much work it would actually require to do in this PR, since I don't really like the idea of leaving this kind of refactoring half complete.

What are your thoughts?

jsight · 2014-09-12T13:12:54Z

I would prefer that we deal with the configuration issue in WINDUP-131.

jsight · 2014-09-12T13:17:37Z

graph/api/src/main/java/org/jboss/windup/graph/GraphContextConfig.java

+    /**
+     * Whether to log a WARNING when initializing the GraphContext lazily.
+     */
+    public boolean isWarnOnLazyInit()


These comments and method names do not appear to match the actual init code. It actually throws and Exception instead of warning.

jsight · 2014-09-12T13:18:39Z

Ok, just minor things remain at this point, IMO. It'd be nice if the commit were at least partially squashed too, IMO.

lincolnthree · 2014-09-12T15:59:49Z

Yeah, I'm not sure how easy it will be to squash this, but I agree it would be realllllly nice to do that. In fact, I think it's worth it, even if it's an hour of pain.

lincolnthree · 2014-09-16T18:47:10Z

Merged in: #251

#close WINDUPRULE-237

lincolnthree reviewed Sep 2, 2014
View reviewed changes

jsight added the Ready to Review label Sep 2, 2014

lincolnthree reviewed Sep 2, 2014
View reviewed changes

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from d70abb7 to 556e069 Compare September 3, 2014 20:02

jsight reviewed Sep 3, 2014
View reviewed changes

OndraZizka added Work in Progress/For Discussion Only and removed Ready to Review labels Sep 3, 2014

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from 556e069 to c4293c7 Compare September 3, 2014 21:23

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from c4293c7 to f47fd06 Compare September 3, 2014 23:11

OndraZizka added 13 commits September 10, 2014 00:33

Javadoc, rename method to better match

5241d71

Move inner classes out

80a7928

Refactor WindupProcessor/Impl to simplify the API, move inner classes…

385462b

… out and offload part of the code like prettyPrintRule() out of this important class.

Refactor WindupProcessor/Impl - move setOutputDirectory()

5915262

Rebase, apply the config in a test, (Applied comments from pull/228 p…

9e0be9c

…art 1)

Rebase, apply the config in a test, comments (Applied comments from p…

b448491

…ull/228 part 2)

WINDUP-272 Refactor GraphContextFactoryImpl and GraphContextImpl - WIP

2aeaf6f

needed by WINDUP-253 Refactor WindupProcessor/Impl

Add config.warnOnLazyInit

a541f0b

WINDUP-272 Refactor GraphContextFactoryImpl and GraphContextImpl - WIP WINDUP-253 Refactor WindupProcessor/Impl

Fix tests

29efe51

WINDUP-272 Refactor GraphContextFactoryImpl and GraphContextImpl - WIP WINDUP-253 Refactor WindupProcessor/Impl

WINDUP-254 Improve error message in RuleSubset. This commit can be re…

eab2029

…moved after development is done.

Trying to fix JavaClassTest...

776aef1

Fix HintsClassificationText

29d2183

GC.init in RuleProviderExecutionStatisticsServiceTest

d2bdc43

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from 78b09f0 to d2bdc43 Compare September 10, 2014 14:29

lincolnthree added 2 commits September 11, 2014 00:39

Merge Ondra's PR

f62a31f

Fix bug causing graph to be re-initialized after configuration

df9ae93

lincolnthree added the Needs Rebase label Sep 11, 2014

Merge pull request #1 from lincolnthree/OndraZizka-WindupProcessor-WI…

f36e6a9

…NDUP-253 Merging the rebase.

lincolnthree removed the Needs Rebase label Sep 12, 2014

Fix WindupArchitectureTest - needs graph init

cfbc64b

jsight reviewed Sep 12, 2014
View reviewed changes

RuleSubset: Information about what happened split between exception a…

1c5cd00

…nd a log message. GCI - don't use confusing new Exception() to get the stack trace. WindupProcessor - code format.

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from f56c36c to 16e33ec Compare September 12, 2014 04:35

Add/fix javadoc.

4b1516d

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from 16e33ec to 4b1516d Compare September 12, 2014 04:37

jsight reviewed Sep 12, 2014
View reviewed changes

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch from cc94d8b to 1dbc7e8 Compare September 12, 2014 16:18

Rename isWarnOnLazyInit to isThrow...

b7157a0

OndraZizka force-pushed the WindupProcessor-WINDUP-253 branch 2 times, most recently from 0dfb08c to b7157a0 Compare September 12, 2014 17:25

lincolnthree closed this Sep 16, 2014

PhilipCattanach pushed a commit to PhilipCattanach/windup that referenced this pull request Feb 20, 2023

Update jboss-eap5and6to7-java-09000 title (windup#228)

27a2697

#close WINDUPRULE-237

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINDUP-253 Refactor WindupProcessor/Impl #228

WINDUP-253 Refactor WindupProcessor/Impl #228

OndraZizka commented Sep 1, 2014

lincolnthree Sep 2, 2014

OndraZizka Sep 3, 2014

OndraZizka Sep 3, 2014

lincolnthree Sep 2, 2014

OndraZizka Sep 3, 2014

OndraZizka Sep 3, 2014

jsight Sep 3, 2014

OndraZizka commented Sep 3, 2014

jsight commented Sep 3, 2014

OndraZizka commented Sep 11, 2014

OndraZizka commented Sep 12, 2014

jsight Sep 12, 2014

lincolnthree commented Sep 12, 2014

jsight commented Sep 12, 2014

jsight Sep 12, 2014

jsight commented Sep 12, 2014

lincolnthree commented Sep 12, 2014

lincolnthree commented Sep 16, 2014

		RecordingWindupProgressMonitor progressMonitor = new RecordingWindupProgressMonitor();
		processor.execute(progressMonitor);


		//<editor-fold defaultstate="collapsed" desc="get/set">

		public Path getOutputDirectory()


		//<editor-fold defaultstate="collapsed" desc="get/set">

		public GraphLifecycleListener getGraphListener()

WINDUP-253 Refactor WindupProcessor/Impl #228

WINDUP-253 Refactor WindupProcessor/Impl #228

Conversation

OndraZizka commented Sep 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraZizka commented Sep 3, 2014

jsight commented Sep 3, 2014

OndraZizka commented Sep 11, 2014

OndraZizka commented Sep 12, 2014

Choose a reason for hiding this comment

lincolnthree commented Sep 12, 2014

jsight commented Sep 12, 2014

Choose a reason for hiding this comment

jsight commented Sep 12, 2014

lincolnthree commented Sep 12, 2014

lincolnthree commented Sep 16, 2014