Skip to content
This repository has been archived by the owner on May 18, 2019. It is now read-only.

Better part path lookup. #50

Merged
merged 1 commit into from
Jan 4, 2014
Merged

Better part path lookup. #50

merged 1 commit into from
Jan 4, 2014

Conversation

JoshTheDerf
Copy link
Collaborator

Includes everything mentioned in #40 (comment) , and uses (a slightly modified version) of the code from #40 (comment)

NEEDS TO BE TESTED ON WINDOWS AND OSX.
It should work perfectly fine on OSX, not so sure about windows.

Also, construction of the paths array should be moved, but it doesn't slow down performance much ATM.

@le717
Copy link
Owner

le717 commented Dec 30, 2013

I haven't tested this on Windows yet, but I can already see a change from reading it.

You can completely remove file_directory_split by using os.path.dirname.

I'm wondering why the space is added at the beginning of the path and is immediately removed. Is this an after effect of file_directory_split And I gather folder paths start with a forward slash on Mac OS X and Linux?

@JoshTheDerf
Copy link
Collaborator Author

Ah, thanks. Does os.path.dirname give the entire path?

And yes, the space is to combat the forward slash at the beginning being removed, and rendering the path invalid. On windows, it should just put in and remove a space.

@le717
Copy link
Owner

le717 commented Dec 31, 2013

Very sorry for the late reply, Tribex.

Entire path meaning everything but the file name? Yes, it should. You can see a bit on how it works in my tutorial on it.

I'm not sure if dirname will remove the introduction forward slash, I don't think it should. I would test it first by opening an interactive Python session and running os.path.dirname() on os.getcwd() and see what happens. If it does remove it, then you may want to put the updated code to add/remove the space under a if sys.platform != "win32": block to prevent it from running on Windows.

@JoshTheDerf
Copy link
Collaborator Author

Sorry, I can't access your website. (I live in China)

Anyhow, I'll do as you suggested, thanks for the tips!

@le717
Copy link
Owner

le717 commented Jan 1, 2014

Aw, well, here is the Gist I have in it as an example. https://gist.github.com/le717/6021206

You're always welcome. Don't ever be afraid to ask. :)

@JoshTheDerf
Copy link
Collaborator Author

Yes sir!

That looks simple enough, simpler than I thought.

<offtopic> Does anyone else think that limiting the length of the lines so much actually hinders readability?
IMHO, it's so much easier to read via shift-scrolling then having to mentally unwrap a line. </offtopic>

Oh, and Happy New Year!

@le717
Copy link
Owner

le717 commented Jan 3, 2014

Patch confirmed to work on Windows, and it looks like it should work on Mac OS X too. If you can not (because of other work) fix it to use os.path.dirname I'll go ahead and merge this and fix it myself (but I'd rather you do so I don't inadvertently break Linux support).

@MinnieTheMoocher Does this all look right to you?

@JoshTheDerf
Copy link
Collaborator Author

I'm a bit too busy to work on it for a few days, but I do want to use the proper way.

@le717
Copy link
Owner

le717 commented Jan 4, 2014

That's alright Tribex. I'll work on it. 😉

Patch merged!

le717 added a commit that referenced this pull request Jan 4, 2014
Better part path lookup, per report from @MinnieTheMoocher
@le717 le717 merged commit 8a422dd into le717:master Jan 4, 2014
@le717 le717 mentioned this pull request Jan 7, 2014
10 tasks
le717 added a commit that referenced this pull request Jan 7, 2014
@le717
Copy link
Owner

le717 commented Jan 7, 2014

@tribex I've updated the patch to use os.path.dirname in bad005d. It should still work on Linux (with the beginning /), though a quick test never hurts. See this for a small visual of what I mean.

@JoshTheDerf
Copy link
Collaborator Author

Thanks, sorry I didn't do it myself. I'm not really free these days until after dinner, when I'm too tired to write any good code.

@JoshTheDerf
Copy link
Collaborator Author

@le717 You have removed the global file_directory variable. That is important!!! file_directory stores the directory of the ldr file you are importing. By changing it to file_name, the current directory becomes the directory of EVERY subsequent part that is attempted to import. Which may cause major problems.

@le717
Copy link
Owner

le717 commented Jan 7, 2014

d3c4685 That better? 😃

@JoshTheDerf
Copy link
Collaborator Author

Getting there 😉

@le717
Copy link
Owner

le717 commented Jan 9, 2014

There we go! 2d588b7 should be correct now.

@JoshTheDerf
Copy link
Collaborator Author

Looks like it :D

@ghost ghost assigned JoshTheDerf Jan 17, 2014
le717 added a commit that referenced this pull request Jan 17, 2014
fix bug with high-/low-res primitives not being imported properly
(builds on #50, #52, #54, use value of `isPart` directly, removed
once-used `file_directory` variable, small cleanup, update Change log
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants