jcabi / jcabi-manifests

Java library for convenient reading of MANIFEST.MF files available in classpath
https://manifests.jcabi.com
Other
60 stars 22 forks source link

URISyntaxException in ClasspathMfs #26

Closed thecrow69 closed 9 years ago

thecrow69 commented 9 years ago

The original implementation which first transforms the resource URLs to URIs and then loop through the URIs to create the streams based on the toURL-method of the URIs leads to a problem in our environment. Our classpath contains spaces and that leads to a URISyntaxException when transforming the URL to an URI. Is there a reason for the double transformation?

dmarkov commented 9 years ago

@thecrow69 I will find a reviewer for your pull requests shortly, thanks for contribution!

dmarkov commented 9 years ago

@dmzaytsev please review

dmzaytsev commented 9 years ago

@thecrow69 CI builds failed please fix it

dmzaytsev commented 9 years ago

@thecrow69 please run Maven build before pushing the changes.

$ mvn clean install -Pqulice

You will see some Checkstyle errors

dmzaytsev commented 9 years ago

@thecrow69 according our [quality rules[ (http://www.teamed.io/qa.html)

Pull request description explains the solution proposed and contains a link to the original ticket it is related to.

please add the link to the original ticket to the pull request description

dmzaytsev commented 9 years ago

@yegor256 I guess there is no ticket for this PR. What we should to do in this case? I think you as the architect should accept the fixing issue before review, isn't it ?

thecrow69 commented 9 years ago

You are right. I haven’t opened a ticket because I needed the source code to find the problem and had an idea how to fix it directly. So I fixed it for us and it worked fine. I wanted to contribute my solution to the community so I created the pull request. Sorry for not reading the guidelines completely before doing this.

Von: Dmitry Zaytsev [mailto:notifications@github.com] Gesendet: Donnerstag, 6. August 2015 12:55 An: jcabi/jcabi-manifests Cc: Hoff, Oliver Betreff: Re: [jcabi-manifests] URISyntaxException in ClasspathMfs (#26)

@yegor256https://github.com/yegor256 I guess there is no ticket for this PR. What we should to do in this case? I think you as the architect should accept the fixing issue before review, isn't it ?

— Reply to this email directly or view it on GitHubhttps://github.com/jcabi/jcabi-manifests/pull/26#issuecomment-128326310.

thecrow69 commented 9 years ago

Hi Dmitry,

I’ve fixed the formal problems. It builds now correctly.

Sorry!

Von: Dmitry Zaytsev [mailto:notifications@github.com] Gesendet: Donnerstag, 6. August 2015 12:31 An: jcabi/jcabi-manifests Cc: Hoff, Oliver Betreff: Re: [jcabi-manifests] URISyntaxException in ClasspathMfs (#26)

@thecrow69https://github.com/thecrow69 please run Maven build before pushing the changes.

$ mvn clean install -Pqulice

You will see some Checkstyle errors

— Reply to this email directly or view it on GitHubhttps://github.com/jcabi/jcabi-manifests/pull/26#issuecomment-128318977.

dmzaytsev commented 9 years ago

@thecrow69 many thanks for your contribution! looks good, but let's wait the architect decision.

dmzaytsev commented 9 years ago

@yegor256 please take a look can we merge it?

yegor256 commented 9 years ago

@rultor try to merge

rultor commented 9 years ago

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor commented 9 years ago

@rultor try to merge

@thecrow69 @yegor256 Oops, I failed. You can see the full log here (spent 14s)

+ rebase=false
+ container=jcabi_jcabi-manifests_26
+ as_root=false
+ mkdir -p /home/rultor/.ssh
+ echo -e 'Host github.com\n\tStrictHostKeyChecking no\n'
+ chmod 600 /home/rultor/.ssh/config
+ git clone git@github.com:jcabi/jcabi-manifests.git repo
Cloning into 'repo'...
+ cd repo
+ git config user.email me@rultor.com
+ git config user.name rultor
+ '[' -z 'sudo gem install pdd' ']'
+ cd ..
+ cat
+ '[' false = true ']'
+ cat
+ chmod a+x entry.sh
+ cat
+ echo 'sudo gem install pdd' ';' 'mvn clean install -Pqulice --errors --settings ../settings.xml' ';' 'mvn clean' ';' 'pdd --source=$(pwd) --verbose --file=/dev/null' ';'
+ rm -rf .gpg
+ cd repo
+ git remote add fork git@github.com:thecrow69/jcabi-manifests.git
+ git remote update
Fetching origin
Fetching fork
From github.com:thecrow69/jcabi-manifests
 * [new branch]      gh-pages   -> fork/gh-pages
 * [new branch]      master     -> fork/master
+ args=
+ '[' default == default ']'
+ args=' --ff'
+ '[' default == no ']'
+ '[' default == only ']'
+ export BRANCH=__rultor
+ BRANCH=__rultor
++ wc -l
++ git show-branch __rultor
+ '[' 0 -gt 0 ']'
+ git checkout fork/master
Note: checking out 'fork/master'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at 08aefdf... Formal code quality problems fixed
+ git checkout -b __rultor
Switched to a new branch '__rultor'
M   src/main/java/com/jcabi/manifests/ClasspathMfs.java
+ git checkout master
error: Your local changes to the following files would be overwritten by checkout:
    src/main/java/com/jcabi/manifests/ClasspathMfs.java
Please, commit your changes or stash them before you can switch branches.
Aborting
yegor256 commented 9 years ago

@thecrow69 you should not touch this line in your branch: https://github.com/jcabi/jcabi-manifests/pull/26/files#diff-ed31ed22e802415e04fd5effd9870314R43

thecrow69 commented 9 years ago

Sorry Yegor, I haven’t touch this line intentionally. Maybe my IDE has done this. I changed it back.

Von: Yegor Bugayenko [mailto:notifications@github.com] Gesendet: Freitag, 7. August 2015 06:51 An: jcabi/jcabi-manifests Cc: Hoff, Oliver Betreff: Re: [jcabi-manifests] URISyntaxException in ClasspathMfs (#26)

@thecrow69https://github.com/thecrow69 you should not touch this line in your branch: https://github.com/jcabi/jcabi-manifests/pull/26/files#diff-ed31ed22e802415e04fd5effd9870314R43

— Reply to this email directly or view it on GitHubhttps://github.com/jcabi/jcabi-manifests/pull/26#issuecomment-128595211.

dmzaytsev commented 9 years ago

@rultor merge

rultor commented 9 years ago

@rultor merge

@dmzaytsev Thanks for your request. @yegor256 Please confirm this.

yegor256 commented 9 years ago

@rultor try to merge

rultor commented 9 years ago

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

rultor commented 9 years ago

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 2min)

dmarkov commented 9 years ago

@dmzaytsev Much obliged! I have added 22 mins to your account in payment "62978483", 120 hours and 8 mins spent. you're getting extra minutes here (c=7). +22 to your rating, your total score is +1699

dmarkov commented 9 years ago

@rultor deploy pls

rultor commented 9 years ago

@rultor deploy pls

@dmarkov OK, I'll try to deploy now. You can check the progress here

rultor commented 9 years ago

@rultor deploy pls

@dmarkov Done! FYI, the full log is here (took me 5min)