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

Latest node version and few small fixes #109

Merged
merged 5 commits into from
Aug 18, 2018

Conversation

avengerweb
Copy link
Contributor

Add support for node 9 & 10
Note: Package node-ffi is maintain, but owner is missing, no one can`tpublish fresh version to npm, there is no releases since 2016.
node-ffi/node-ffi#465 (comment)
Updated readme for linux build
Fixed problems on case sensetive systems with chain js (face with it when test your DB api) I tested on my custom client, where patches named randomly

Note: Package node-ffi is maintain, but owner is missing, no one can`t
publish fresh version to npm
Update readme add information related with ubuntu and debian OS
Copy link
Member

@timkurvers timkurvers left a comment

Choose a reason for hiding this comment

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

Left a few comments that need to be addressed.

As for package-lock.json, could you generate it using npm 5.x?

In the future, please open separate PRs for each related set of changes.

Thanks again, much appreciated!

.gitignore Outdated
@@ -3,3 +3,4 @@
/node_modules/
/spec/**/*.js
*.dll
.idea
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx

- '7'
- '8'
- '9'
- '10'
Copy link
Member

Choose a reason for hiding this comment

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

👍

README.md Outdated
@@ -81,6 +81,16 @@ to `libblp.dll` and ensure it ends up on the load PATH.
By default, node-gyp compiles ffi for x64 so make sure `libblp.dll`
matches this architecture.

#### Ubuntu, Debian
```shell
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a newline between the heading and the code snippet?

README.md Outdated
$ cmake CMakeLists.txt -DWITH_LIBRARY=YES
$ sudo make install
$ sudo ldconfig
```
Copy link
Member

Choose a reason for hiding this comment

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

Great to finally have this in place, thanks! It'll resolve issue #88.

Could you remove the $-indicator on each line so that it can be easily copy pasted?

@@ -27,7 +27,7 @@ class MPQChain {
path.join(dataDir, mpq)
));

const archives = glob.sync(patterns);
const archives = glob.sync(patterns, {nocase: true});
Copy link
Member

Choose a reason for hiding this comment

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

For which scenario is this a requirement? Are MPQs shipped with variants in casing?

Copy link
Member

Choose a reason for hiding this comment

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

Make sure to run npm run lint to comply with the linting configuration for this project. In this case, we require spacing within object literals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soo, for example from torrent sometime I get with random case :) Mostly it for custom clients, I help for some private server project, which has big count of client changes (races, quests, spells, models and etc) and you database API is helpful for it (but not working properly at this moment, custom races not properly show and quests eat CPU and memory but nothing shown) and yes, all mpq`s in lower case

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. If you could add the spacing, that'd be great 👍

As for custom races not properly showing and quests consuming CPU / memory, please open separate issues. There's likely things that can be done there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make changes for you review in 15-20 hours, have some other work.

Fix code style (lint)
Remove unneeded line from .gitignore
Regenerate package json using npm 5.10
Copy link
Contributor Author

@avengerweb avengerweb left a comment

Choose a reason for hiding this comment

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

Check it

@timkurvers timkurvers merged commit c40ce29 into wowserhq:master Aug 18, 2018
@timkurvers
Copy link
Member

Looks good, thanks! 👍

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