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

JBIDE-12821 #134

Merged
merged 1 commit into from Sep 9, 2013
Merged

JBIDE-12821 #134

merged 1 commit into from Sep 9, 2013

Conversation

robstryker
Copy link
Member

Adds regexp support for both project archives and astools publishing via the component model.

* @version $Id$
*/
public class StringUtils
{
Copy link
Member

Choose a reason for hiding this comment

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

can't you use commons-lang's StringUtils class instead in your imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will look into this idea.

@maxandersen
Copy link
Member

I understood it as you've made this work with eclipse VFS file system - I don't see that. I see you made it possible to abstract the traversal of the filesystem but that this filesystem must map to java.io.File's elements. Thus it is not really working with the Virtual file system (i.e. how would this thing here work with linked resources in eclipse workspace ?)

And just in general please avoid non-informative commits like:

  • JBIDE-12812 initial commit
    • JBIDE-12812 fixed a bug
    • JBIDE-12812 more coding
    • JBIDE-12812 yet more dev work / organization / jdoc

Squash that and actually explain/document what the commit does. Above is not relevant for anyone but you working locally - in a review case and going back to find the changes/track down why something happened those commit messages will just be noise.

@robstryker
Copy link
Member Author

I understood it as you've made this work with eclipse VFS file system - I don't see that.

The project archives client / subclass used a FileWrapper, which is part of API and is also part of method signatures and so cannot be changed. You can see it here:

https://github.com/robstryker/jbosstools-server/blob/8d708a8c29c2c775c823c45bd88b80d9e9017652/archives/plugins/org.jboss.ide.eclipse.archives.core/src/main/org/jboss/ide/eclipse/archives/core/model/DirectoryScannerFactory.java#L195

The lines you'll be looking at are: protected File[] list2workspace(File file) , which is where it uses the VFS to return the list of children for any given workspace File. The strange thing here is that archives.core ALSO does not know about eclipse classes except for very very few, due to the original structure 6 years ago, so they also cannot traverse the IResource model directly. Strangely enough, I could import them into the file (they are recognized as on the classpath) but it breaks the very old ant use-case and requires more jars being on teh classpath when running standalone.

So the structure of project archives is super old and somewhat fragile. It is very easy to accidentally import eclipse classes and then have the ant usecase broken.

(i.e. how would this thing here work with linked resources in eclipse workspace ?)

Linked resources do work. They get sent to a class named WorkspaceVFS:

https://github.com/robstryker/jbosstools-server/blob/8d708a8c29c2c775c823c45bd88b80d9e9017652/archives/plugins/org.jboss.ide.eclipse.archives.core/src/eclipse/org/jboss/ide/eclipse/archives/core/model/other/internal/WorkspaceVFS.java



/**
* Get a list of child files from this file.
Copy link
Member

Choose a reason for hiding this comment

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

"Get a list of child files or directories", right?

@dgolovin
Copy link
Member

dgolovin commented Sep 4, 2013

running all tests from jbosstools-server/archives/tests shows 0% coverage for new plugin

@robstryker
Copy link
Member Author

It seems very strange to me that running the tests gets 0% coverage. The test suite very explicitly adds 2 new tests which I have personally seen run.

    suite.addTestSuite(DirectoryScannerRegexTest.class);
    suite.addTestSuite(DirectoryScannerModelTest.class);

Can you please outline your methodology? I find it very difficult to imagine these two tests have 0% coverage, and it's clear in the PR that the two tests have been added, and if you read the (fairly simple) tests, they clearly import, instantiate, and use, the classes in the new plugin.

@dgolovin
Copy link
Member

dgolovin commented Sep 4, 2013

Coverage problem is related to parent pom and archives.scanner properties, I'll submit PR with temporary fix for your branch

@dgolovin
Copy link
Member

dgolovin commented Sep 4, 2013

See in robstryker#2 how to fix coverage report for archives tests

@dgolovin
Copy link
Member

dgolovin commented Sep 4, 2013

Would be good to have origin reference for every 3rd party class copy and enhancements/changes description

as converting existing clients to use the new bundle

JBIDE-12821 adding 2 tests

JBIDE-12821 - rewrote the scanners to use generics and interfaces

JBIDE-12821 - removed unnecessary .asf packages and split things into simply scanner and scanner.internal

JBIDE-12821 - removed unnecessary .asf packages and split things into simply scanner and scanner.internal. Apparently the files did not 'move' properly

JBIDE-12821 - bad manifest.mf line, erroneous @OverRide line

JBIDE-12821 - adding new bundle for scanners, and new feature, and updating existing features to ensure its present

JBIDE-12821 - renaming package since it did not match bundle name

JBIDE-12821 - fixed failing unit test and small problems with feature version etc

JBIDE-12821 - updating javadoc and removing from sysouts from tests

JBIDE-12821 - fixing version to match newest archives versions

JBIDE-12821 updating manifest.mf for version ranges

JBIDE-12821 somehow missed adding the entire source folder

JBIDE-12821 - reverting changes to org.jboss.ide.eclipse.archives.core.asf.DirectoryScanner to maintain api compatability during deprecation.

JBIDE-12821 - copyright date updates

JBIDE-12821 renamed one method which referenced file instead of node, updated javadoc

[JBIDE-12821] change code coverage  configuration for archives module

new plugin included in coverage report:
org.jboss.tools.archives.scanner

JBIDE-12821 - removal of stringutils as unnecessary

JBIDE-12821 - further removal of all unneeded methods to ensure the smallest plugin possible

JBIDE-12821 - modifying the code in DirectoryScannerFactory to ensure the smallest API breakage possible. Replacing all moved or missing method signatures

JBIDE-12821 - added an api compatability filter; usage scan reveals no outside consumers
@robstryker robstryker merged commit 116b17f into jbosstools:master Sep 9, 2013
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