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

add 'q KDB' package #5156

Merged
merged 2 commits into from
Feb 19, 2016
Merged

add 'q KDB' package #5156

merged 2 commits into from
Feb 19, 2016

Conversation

komsit37
Copy link
Contributor

No description provided.

@FichteFoll
Copy link
Collaborator

  1. Note that __init__.py will be loaded twice because of a ST bug (?).
  2. When you include styled_popup directly and not use it as a dependeny (for whatever reason), you must not prepend it to sys.path because that will either cause all other packages to use your package or the module will have been loaded already anyway and you'll use the dependency anyway.
    Either use the actual dependency or relative imports.
  3. Maybe you just want to provide numpy as a dependency instead? It is the ideal use case. You would only need sys.path for qpython then.
  4. You have several .pyc files in you repo.
  5. In build systems "$file_path/$file_name" is equivalent to "$file". Your env there is also purely for UNIX, so you might want to consider using platform-specific overrides: http://docs.sublimetext.info/en/latest/reference/build_systems/configuration.html#platform-specific-options
  6. I suppose most of your methods in the Settings class would be better used as classmethods instead of staticmethods.
  7. If including a generally wide-spread command like chain, you should consider prefixing it in a way that will not override potential user modifications to that commands.

@komsit37
Copy link
Contributor Author

Thanks for your comments. I will try to fix all of them.

@komsit37
Copy link
Contributor Author

raised #5168 for adding numpy dependency in a separate package

@komsit37 komsit37 mentioned this pull request Jan 27, 2016
@FichteFoll
Copy link
Collaborator

Similarly to the numpy dependency PR, you should restrict the OS architecture in your platform specifications.

@komsit37
Copy link
Contributor Author

komsit37 commented Feb 7, 2016

Fixed all above issues in master https://github.com/komsit37/sublime-q
Let me know if you have anything else you want me to fix

@komsit37
Copy link
Contributor Author

One more task

  • restrict the OS architecture in your platform specifications

FichteFoll added a commit that referenced this pull request Feb 19, 2016
@FichteFoll FichteFoll merged commit 64abde3 into wbond:master Feb 19, 2016
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.

2 participants