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

Adding luis:application:import cmd #367

Merged
merged 9 commits into from Nov 20, 2019
Merged

Conversation

jspruance
Copy link

No description provided.

@jspruance
Copy link
Author

  • added luis:application:import cmd
  • updated bf 'command' to expose stdin reading function
  • disregard changes to 'luis:application:create' here - this was a 'partial handcrafted artisnal merge' so that it would work with latest pending changes

@@ -5,5 +5,6 @@

import {CLIError, Command, flags} from './command'
import utils from './utils'
export {CLIError, Command, flags, utils}
import ReadPipedStdin from './readpipeddata'
Copy link
Member

Choose a reason for hiding this comment

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

This is no needed, we are exposing this functionality as part of the command class. Please refer to luis:convert to see an example.

Copy link
Author

Choose a reason for hiding this comment

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

refactored. after looking at it though, i still think it should live in utils because we have all other read / write / parse functionality there, so now there is this one-off. maybe there's a deeper inconsistency with how we achieve code reuse - through class inheritance vs utility functions. maybe we should have done it all one way or another. anyways...this works for now.

@@ -18,6 +18,20 @@ const filterConfig = (config: any, prefix: string) => {
}, {})
}

const getInputFromFileOrStdin = async (args: any): Promise<string> => {
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we have to do this here

Copy link
Author

Choose a reason for hiding this comment

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

refactored

@@ -55,6 +69,10 @@ const processInputs = async (flags: any, flagLabels: string[], configDir: string
flagLabels
.filter(flag => flag !== 'help')
.map((flag: string) => {
if (flag === 'in') {
Copy link
Member

Choose a reason for hiding this comment

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

Should we exclude this flag?

Copy link
Author

Choose a reason for hiding this comment

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

meaning remove it from input processing in utils? i don't think so - i think we should stay consistent

Copy link
Author

Choose a reason for hiding this comment

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

leaving as is

Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

CIL

}

async getImportJSON(input: string) {
if (input) {
Copy link
Member

Choose a reason for hiding this comment

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

Stdin should override flags.in

Copy link
Member

Choose a reason for hiding this comment

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

If no stdin or flags.in value passed, CLIError should be thrown asking the user to provide input

Copy link
Author

Choose a reason for hiding this comment

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

added error & unit test

@jspruance
Copy link
Author

fixes #367

Copy link
Member

@munozemilio munozemilio left a comment

Choose a reason for hiding this comment

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

CIL

@munozemilio munozemilio merged commit 8700bd0 into master Nov 20, 2019
@munozemilio munozemilio deleted the 306-luis-application-import branch November 20, 2019 20:45
@WTobor
Copy link

WTobor commented Nov 28, 2019

Hi! When this changes will be available on npm?

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.

None yet

3 participants