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

Contributed library's import statements now gets added correctly #3404

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@joelmoniz
Copy link
Member

joelmoniz commented Jun 19, 2015

Adding in import statements corresponding to a contributed library via Sketch>Import Library...>(contributed library) now adds in the correct import statement(s). The error was because of the relatively new specifiedImports field in the .properties file, which is blank for most library contributions at present. The import statements were getting read from here, and null was being returned only if the field was null. However, the field was being parsed and stored as an empty string if the field was not present/was blank, which caused an import statement of the form import (blank string).* to be added.

This resolves #3403.

@benfry

This comment has been minimized.

Copy link
Member

benfry commented Jun 19, 2015

We should fix this at the source, not add another band-aid for the symptom. Why is a blank entry ever being added to the list in the first place?

@joelmoniz joelmoniz force-pushed the joelmoniz:fixImport branch from 599bb4e to 7974d4c Jun 19, 2015

@joelmoniz

This comment has been minimized.

Copy link
Member

joelmoniz commented Jun 19, 2015

Ah, sorry about that. This happens because of the specifiedImports field in the library .properties file. It is a relatively new field, and is left blank in most of the contributed libraries, as a result of which a blank entry gets stored. I've now updated the commit to ensure that the blank entry be detected and treated as missing (stored as a null) during the initial parsing of the .properties file itself, instead of handling this check in the getSpecifiedImports() method like I had done till now.

@benfry

This comment has been minimized.

Copy link
Member

benfry commented Jun 22, 2015

It doesn't look like you've updated the PR.

@joelmoniz

This comment has been minimized.

Copy link
Member

joelmoniz commented Jun 22, 2015

The PR seems to be updated. The change was originally in getSpecifiedImports(), but is now in the correct location parseImports(). The fix itself is the same, however, just that it is now added at the correct place.

@benfry

This comment has been minimized.

Copy link
Member

benfry commented Jun 22, 2015

Fixed through another route, plus other major cleanups in there.

@benfry benfry closed this Jun 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment