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

Update controller.rb #77

Merged
merged 3 commits into from
Aug 12, 2014
Merged

Update controller.rb #77

merged 3 commits into from
Aug 12, 2014

Conversation

illtellyoulater
Copy link
Contributor

fixed to return a valid path for a file in the root of a jar

fixed to produce a valid path for a file in a jar root
@byteit101
Copy link
Member

What about "" (empty string)? Also we might want "/" to work also, do you have any opinions @enebo ?

@illtellyoulater
Copy link
Contributor Author

I tried that before. File.join("","file.txt") => "/file.txt" so no good.
Concerning "/", a || condition could be added to the check for "."
Is it possible to update the pull request or do I have to delete and make a new one?

@amiracam
Copy link

amiracam commented Dec 3, 2013

I'm sorry what I missed something what is this accomplishing ? Is this about not being able to load the fxml file ?

@illtellyoulater
Copy link
Contributor Author

yes, when running from a jar, for a .fxml file living in the jar root, defined by the second argument of fxml_root in mair script with ("fxml_root filesystem/path", ".")

@edubkendo
Copy link
Member

@jjgh any commits you push to the branch your PR was on will automatically be added to the PR.

@amiracam
Copy link

amiracam commented Dec 3, 2013

ok, I see, in my case I used :

fxml_root File.dirname(FILE), 'src/lib'

the way I packed things up my code is under /src at root of jar, the only thing right on root is JarMain.class which would be part of warbler's packaging

src/bin
is where the launcher class resides wherein I add src/lib to load path
I then require my app

I suppose it provides for more flexibility to be able to grab fxml file from root, but I generally would not want to pack that way and its certainly not the defaults warbler likes

@byteit101
Copy link
Member

ah, no, I was saying should we check for "" and "/" and if so don't append either?

@illtellyoulater
Copy link
Contributor Author

if you are asking to me, yea that is what I tried to say when you first asked :)
@amiracam maybe it sounds more reasonable to place .fxml files somewhere else, but I would just like avoid someone else a big headache if by chance they choose to use the root folder.

@amiracam
Copy link

amiracam commented Dec 4, 2013

Sure , I'm also suggesting it due to warbler
On Dec 4, 2013 7:53 AM, "jjgh" notifications@github.com wrote:

if you are asking to me, yea that is what I tried to say when you first
asked :)
@amiracam https://github.com/amiracam maybe it sounds more reasonable
to place .fxml files somewhere else, but I would just like avoid someone
else a big headache if by chance they choose to use the root folder.


Reply to this email directly or view it on GitHubhttps://github.com//pull/77#issuecomment-29802095
.

@enebo
Copy link
Member

enebo commented Dec 4, 2013

HEH, "/". The pedantic side of me realizes that is someone wants their fxml in / of their machine it will stop working if they are trying to load from a jar file :) I would say the check should be a reasonably named method or there should be a simple comment explaining the rationale. I guess I don't care one way or the other on "/".

@byteit101
Copy link
Member

Actually, when I think about it, "" should do this, but "." and "/" should (eventually) do file paths.

@enebo
Copy link
Member

enebo commented Dec 10, 2013

Yeah I guess that was the gist of my pedantry...

@byteit101
Copy link
Member

@jjgh mind making "" do jars and "." and "/" do files? (I can fix it myself if you don't mind, though I might not be able to do it given the immense scale of your patch 😉 )

Did I get it right?
Did I get any close to what you asked? :)
byteit101 added a commit that referenced this pull request Aug 12, 2014
@byteit101 byteit101 merged commit 1bcf8fd into jruby:master Aug 12, 2014
@illtellyoulater
Copy link
Contributor Author

Thanks for merging it, it was my first commit into a big open source project! Great :)

@illtellyoulater illtellyoulater deleted the patch-1 branch August 20, 2014 11:19
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

6 participants