Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

LU refactor#717

Merged
boydc2014 merged 16 commits intomasterfrom
donglei/lu-refactor
Aug 23, 2019
Merged

LU refactor#717
boydc2014 merged 16 commits intomasterfrom
donglei/lu-refactor

Conversation

@boydc2014
Copy link
Contributor

@boydc2014 boydc2014 commented Aug 19, 2019

Description

As describe in this issues #716, recently changes into lu workflow add new features but break some interfaces and conventions. We need clean up and keep the code in a good shape before moving forward.

This refactor don't add\remove any feature. You can see, we -466 code, +155 code, and achieve the same thing in a more readable way. And enabled some tests, which is previously skipped.

Task Item

Please include the link to the related work item, like fix Something is not working

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@boydc2014 boydc2014 marked this pull request as ready for review August 21, 2019 04:19
@boydc2014 boydc2014 requested review from lei9444 and liweitian August 21, 2019 04:19
Copy link
Contributor

@liweitian liweitian left a comment

Choose a reason for hiding this comment

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

LGTM

await this.luIndexer.flush(this.files, relativePath);
return this.luIndexer.getLuFiles();

// TODO: valid before save
Copy link
Member

Choose a reason for hiding this comment

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

s/valid/validate - will this happen in this change?

Copy link
Contributor Author

@boydc2014 boydc2014 Aug 22, 2019

Choose a reason for hiding this comment

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

no, this don't have validate before, so i don't include that work in this refactor, i will do that in another PR, i will create a validation interface for this, to consolidate the validation done for dialog\lu\lg.

@boydc2014
Copy link
Contributor Author

@a-b-r-o-w-n Thanks for fixing the grammar and typos, i have a long way to go with my English :)

boydc2014 and others added 2 commits August 22, 2019 11:16
* handle comments

* fix test case

* fix test cases
@cwhitten cwhitten dismissed stale reviews from a-b-r-o-w-n and themself August 23, 2019 03:07

stale

@boydc2014 boydc2014 merged commit e88a3e0 into master Aug 23, 2019
@boydc2014 boydc2014 deleted the donglei/lu-refactor branch August 23, 2019 03:07
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.

4 participants