Skip to content

Conversation

@zhengbli
Copy link
Contributor

Refactor the overall structure of the codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Make these loops terminate early?

@zhengbli
Copy link
Contributor Author

There is one catch that the line, offset on the server are 1-based, while the line, column used by sublime are 0-based. It might be clearer to refactor the names to one_based_line and zero_based_line.

@billti
Copy link
Member

billti commented May 18, 2015

We seem to be inconsistent on the file naming. Some have capitals in them (e.g. "TypeScript.py"), some have underscores (e.g. "go_to_type.py" or "quick_info.py"), and some don't (e.g. "jsonhelpers.py" or "eventhub.py"). It would be good to make this consistent while doing all the work to move files around now. Personally, I like all lower case with underscores, e.g. "quick_info.py", which aligns with the PEP8 guidance. (p.s. "auto_indent_on_enter_between_curly_braces.py" is probably a little long. Can we shorten this?)

Also, why did we make 'commands' and 'listeners' top level packages also, rather than residing within one top level package (e.g. 'libs')?

@zhengbli
Copy link
Contributor Author

I agree that all lower case with underscore file names would make more sense.
I don't really have a preference regarding weather to make the 'commands' and 'listeners' top level packages or not, would that make a difference? Both anaconda and omnisharp are doing it too, so I didn't give it much thought.

@billti
Copy link
Member

billti commented May 18, 2015

It's not a big deal about the top-level thing, so we can leave it as is for now. If everything was surfaced in the top-level package it would allow you to just have "from libs import *" in the typescript.py, but again, as-is currently is fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several of these lines go way beyond the 79 char width recommended in PEP8. You can use parens to group imports and not need the line continuation character. For example, the above might look better as:

from .format import (
    TypescriptFormatBrackets, 
    TypescriptFormatDocument, 
    TypescriptFormatLine, 
    TypescriptFormatOnKey,
    TypescriptFormatSelection, 
    TypescriptPasteAndFormat)

zhengbli added a commit that referenced this pull request May 19, 2015
@zhengbli zhengbli merged commit 2d3d522 into microsoft:master May 19, 2015
@ddotlic
Copy link

ddotlic commented May 20, 2015

FYI: you broke a few things by renaming jsonhelpers to json_helpers but not updating all the places where this is used. Example: on lines 19 and 74 in commands/references.py you still call jsonhelpers but should call json_helpers, this breaks "Find All References"; I've fixed locally and verified that it works.

@zhengbli
Copy link
Contributor Author

@ddotlic Oops I just noticed as well. Will correct in a minute. Thanks for the heads up!

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.

6 participants