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

added option to change known properties at runtime #35

Closed
wants to merge 1 commit into from
Closed

added option to change known properties at runtime #35

wants to merge 1 commit into from

Conversation

SirkoS
Copy link
Contributor

@SirkoS SirkoS commented Aug 16, 2019

this adds the option to set the properties used by wikibase-edit within the runtime configuration instead of having to update the dependency manually.

when working with other wikibase instances I guess this makes it easier to maintain a custom set of properties without having to fiddle around in the dependencies.

@maxlath
Copy link
Owner

maxlath commented Aug 16, 2019

hey @SirkoS, thanks for this PR! unfortunately, there is already a branch addressing this issue, and more generally getting rid of wikidata.org coupling and renaming the module wikibase-edit. The approach taken by this branch is to query properties datatypes only when needed, and directly from the Wikibase API: even less configuration to do \o/ could you give it a try?
NB: there are several yet undocumented breaking changes, so you can also just wait for the new version to be documented and published

@SirkoS
Copy link
Contributor Author

SirkoS commented Aug 16, 2019

I missed that. I just saw some comments here and there about that split, but didn't spot the branch.

I'll try to test that branch with my code. Any ETA for when you'll publish that version will be available via npm?

@maxlath
Copy link
Owner

maxlath commented Aug 16, 2019

@SirkoS hopefully in the coming week

@SirkoS
Copy link
Contributor Author

SirkoS commented Aug 16, 2019

Sounds good!

I was able to rewrite my code using you branch. So from my side everything seems fine.

While looking through your code for the mentioned changes, I noted some things:

if (config.credentials.oauth && (config.credentials.username || config.credentials.password)) {
  • you currently rely on your own (?) request wrapper bluereq. why not just use the "official" one request-promise-native? This would also remove the (indirect) bluebird dependency. As you use native promises on other occasions already, there is no need for bluebird in my eyes.

  • following the previous note: if you bump the supported node version a little, you could make use of asnyc/await, which would also simplify the code in my eyes.

@maxlath
Copy link
Owner

maxlath commented Aug 16, 2019

  • validateAndEnrichConfig has been fixed in 5b1b634
  • there is no official request wrapper, just different public relations strategies ;) that said, I agree we should move to something else
  • not using async/await means no transition work and better support for older NodeJS versions, so while I see the readability benefit, I would like to keep procrastinating a bit on this :)

@maxlath
Copy link
Owner

maxlath commented Aug 17, 2019

@SirkoS wikibase-edit v3.0.1 has been published! see changelogs

@SirkoS
Copy link
Contributor Author

SirkoS commented Aug 20, 2019

@SirkoS wikibase-edit v3.0.1 has been published! see changelogs

Great!
I'm sorry about abusing this thread and my constant nagging, but I found two other minor issues:

  • in the changelog the link to "create property" uses the wrong hash right now (looks like a c&p error)
  • the npm banner in the README.md shows the wikibase-sdk data and not the one from wikibase-edit (the link is fine, however)

(I'll close the request here)

@SirkoS SirkoS closed this Aug 20, 2019
maxlath added a commit that referenced this pull request Sep 14, 2019
@maxlath
Copy link
Owner

maxlath commented Sep 14, 2019

@SirkoS changelog and banner fixed, thanks for reporting!

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

2 participants