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

Support generating an empty extension #54 #55

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

JonasHelming
Copy link
Contributor

@JonasHelming JonasHelming commented Apr 6, 2020

Description
This PR adds a new template to the generator, which is provides an empty template. This replaces the boolean option "example" which allowed to generate the templates without contributions, which was broken / led to not compilable code

How to test:

clone the repository, and checkout the appropriate branch
perform yarn
perform npm link (creates local copy instead of getting from npm directly)
create a directory on your filesystem and cd into it
perform yo theia-extension
select the LabelProvider empty

removed option "example", added dedicated empty template

Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
@vince-fugnitto
Copy link
Member

@JonasHelming I'm sorry I haven't reviewed earlier, I completely forgot about the ping.

@JonasHelming
Copy link
Contributor Author

@JonasHelming I'm sorry I haven't reviewed earlier, I completely forgot about the ping.

No worries! :-)

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified and the empty template option works well.
I did notice a bug with the label provider however (not related to your changes):

lerna ERR! src/browser/a-contribution.ts(22,5): error TS2416: Property 'getIcon' in type 'ALabelProviderContribution' is not assignable to the same property in base type 'LabelProviderContribution'.
lerna ERR!   Type '() => MaybePromise<string>' is not assignable to type '(element: object) => string | undefined'.
lerna ERR!     Type 'MaybePromise<string>' is not assignable to type 'string | undefined'.
lerna ERR!       Type 'Promise<string>' is not assignable to type 'string'.
lerna ERR! info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR!
lerna ERR!     at Promise.all.then.arr (/Users/vincentfugnitto/Desktop/test/vince/node_modules/execa/index.js:236:11)

@JonasHelming
Copy link
Contributor Author

I verified and the empty template option works well.
I did notice a bug with the label provider however (not related to your changes):

lerna ERR! src/browser/a-contribution.ts(22,5): error TS2416: Property 'getIcon' in type 'ALabelProviderContribution' is not assignable to the same property in base type 'LabelProviderContribution'.
lerna ERR!   Type '() => MaybePromise<string>' is not assignable to type '(element: object) => string | undefined'.
lerna ERR!     Type 'MaybePromise<string>' is not assignable to type 'string | undefined'.
lerna ERR!       Type 'Promise<string>' is not assignable to type 'string'.
lerna ERR! info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR!
lerna ERR!     at Promise.all.then.arr (/Users/vincentfugnitto/Desktop/test/vince/node_modules/execa/index.js:236:11)

When exactly did that happen? If you can provide the details, maybe as a separate ticket, I am happy to fix this.

@JonasHelming
Copy link
Contributor Author

@vince-fugnitto : Do you see any chance to merge this any time soon? I want to look at the other issue you reported and I have another fix I want to work on, but both kind of depends on this, so it would be good to know if you plan to merge this? Sorry for pushing!

@vince-fugnitto
Copy link
Member

@vince-fugnitto : Do you see any chance to merge this any time soon? I want to look at the other issue you reported and I have another fix I want to work on, but both kind of depends on this, so it would be good to know if you plan to merge this? Sorry for pushing!

Sorry @JonasHelming, I don't see the pings from this repo well!
I'm fine with merging the pull-request, once the other issue is resolved we can create a release.
I'll re-test the failure so I can easily document it in an issue :)

@vince-fugnitto vince-fugnitto merged commit 5879f4b into eclipse-theia:master Apr 29, 2020
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