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

Convert Directory and File classes to JavaScript #1226

Merged
merged 10 commits into from Mar 6, 2018

Conversation

Projects
None yet
2 participants
@50Wliu
Member

50Wliu commented Feb 23, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Converts the Directory and File-related classes to JavaScript. I plan on investigating switching tree-view over to @atom/watcher, which the JavaScript conversion will help with.

Alternate Designs

None.

Benefits

JavaScript!

Possible Drawbacks

Potential regressions. I'd like to think that spec coverage is pretty good for tree-view but I'll be using this branch for a while before merging to play it extra safe.

Applicable Issues

None.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Feb 23, 2018

Member

I considered doing this too but I wondered if it'd totally break a bunch of pending PRs. Is there anything we absolutely want to merge before doing this?

Member

daviwil commented Feb 23, 2018

I considered doing this too but I wondered if it'd totally break a bunch of pending PRs. Is there anything we absolutely want to merge before doing this?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 24, 2018

Member

@daviwil Thanks for bringing that up. I've identified the following PRs that also modify one of the files changed in this PR:
#1144
#1048
#955 (already conflicting)
#744 (already conflicting)
#658 (already conflicting)
#614 (already conflicting)
#469 (already conflicting)
#344 (already conflicting)

Therefore I believe the ones we should look at and potentially merge before this one are #1144 and #1048. It looks like @lee-dohm is already helping with #1048, which only leaves #1144.

Member

50Wliu commented Feb 24, 2018

@daviwil Thanks for bringing that up. I've identified the following PRs that also modify one of the files changed in this PR:
#1144
#1048
#955 (already conflicting)
#744 (already conflicting)
#658 (already conflicting)
#614 (already conflicting)
#469 (already conflicting)
#344 (already conflicting)

Therefore I believe the ones we should look at and potentially merge before this one are #1144 and #1048. It looks like @lee-dohm is already helping with #1048, which only leaves #1144.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Feb 28, 2018

Member

Just merged #1144 and #1048! Proceed :)

Member

daviwil commented Feb 28, 2018

Just merged #1144 and #1048! Proceed :)

50Wliu added some commits Feb 23, 2018

@50Wliu 50Wliu merged commit fff1679 into master Mar 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-directory-file-js branch Mar 6, 2018

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