-
Notifications
You must be signed in to change notification settings - Fork 30
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
Improving the published package #39
Comments
Yes, I would accept that. I remember having to do something a little hacky to access the TypeScript definitions, so I can understand the annoyance. Unfortunately I don't think Victor has type definitions. |
Victor is quite old, it does actually cause a few headaches.. eg doesn't expose a default on its module exports. To get it to compile properly, I think you need to add a Someone has also made types for it, in @types/victor I have made a start on a branch https://github.com/blacha/factorio-blueprint/tree/chore/adding-build-pipeline I also don't really have any preferences on linting/formatting.. So if you have any strong opinions on tabs vs spaces, single vs double quotes I can change it. |
I would say just don't change a lot (e.g. Keep spacing as 2, keep I'm not attached to Victor, but I'm not sure if it's the best idea to write our own implementation (since we want to do more than just store the value—we add/subtract from it, multiply, clone, etc). I've looked a little but I can't seem to find any alternatives. Not sure exactly what to do. |
Awesome, My goal is not to change anything if i can avoid it, Just to make sure its standard across all the files so my editor doesn't keep auto-formatting things causing my commits to get too large. By just adding a base prettier config, it doesn't really change very much, it is mostly just multi lining long statements, I was going to look at making the default line length longer to reduce the amount of changes, Here is the base commit so far: https://github.com/blacha/factorio-blueprint/commit/1be1c58804f23f6c462503318baec75545897f20 This is a work in progress, I am looking to reduce the amount of changes. Its mostly just removing excess commas, adding/removing Victor seems fine as a library, just needs a little bit of work to get it going in Typescript/nodejs land, I have a build working with nodejs compiled as commonjs modules. So might be able to give you a couple PRs tomorrow. I will split them into a format/linting PR + the node build PR |
Trailing commas are a good feature that makes editing easier and reduces diff churn, and Prettier allows enabling them in its config, and it only has them off by default for compatibility reasons that aren't relevant here. |
I am neither for or against trailing commas. Most of the codebase does not have trailing commas. So to reduce the size of the PR I left them off. |
MR size is a legit point, but removing the existing trailing commas, referring to them as "excess", and setting up automation that enforces their removal isn't a neutral action. |
I was looking at using this library for a project I am working on, however the published package makes it kind of difficult.
Would you accept a pull request to update the build process?
Key areas of improvement:
Also running the build in the CI system would be helpful especially if it had some sort of formatting (prettier/other) and linting (eslint/tslint ?) configuration.
The text was updated successfully, but these errors were encountered: