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.

Sign up for GitHub

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

Jump to bottom

fix value parse check #99

Merged
merged 6 commits into from
Jun 22, 2019
Merged

fix value parse check #99

merged 6 commits into from
Jun 22, 2019

Conversation

Diluka
Copy link
Contributor

@Diluka Diluka commented Jun 13, 2019 •
edited
Loading

this fix #97 close #49
value has been parsed correctly, but check only length. Numbers and Booleans don't have length.

refactor #104 and add ISO-8601 support

@Diluka Diluka requested a review from michaelyali June 13, 2019 08:48
@michaelyali
Copy link
Member

@Diluka could you please rebase, because of #100
Also, you've removed js files as far as I can see, that's why tests are failing.
run yarn s build.{package-name}, like this:
yarn s build.crud
yarn s build.crud-request
yarn s build.crud-typeorm
yarn s build.crud-util

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

@zMotivat0r the question is I can't execute the build task. yesterday I can ┑(￣Д ￣)┍

error TS6059: File '/Users/Diluka/github/crud/packages/util/src/checks.util.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request'. 'rootDir' is expected to contain all source files.
error TS6059: File '/Users/Diluka/github/crud/packages/util/src/checks.util.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request/src'. 'rootDir' is expected to contain all source files.
error TS6059: File '/Users/Diluka/github/crud/packages/util/src/index.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request'. 'rootDir' is expected to contain all source files.
error TS6059: File '/Users/Diluka/github/crud/packages/util/src/index.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request/src'. 'rootDir' is expected to contain all source files.
error TS6059: File '/Users/Diluka/github/crud/packages/util/src/obj.util.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request'. 'rootDir' is expected to contain all source files.
error TS6059: File '/Users/Diluka/github/crud/packages/util/src/obj.util.ts' is not under 'rootDir' '/Users/Diluka/github/crud/packages/crud-request/src'. 'rootDir' is expected to contain all source files.
error TS6307: File '/users/diluka/github/crud/packages/util/src/checks.util.ts' is not in project file list. Projects must list all files or use an 'include' pattern.
error TS6307: File '/users/diluka/github/crud/packages/util/src/index.ts' is not in project file list. Projects must list all files or use an 'include' pattern.
error TS6307: File '/users/diluka/github/crud/packages/util/src/obj.util.ts' is not in project file list. Projects must list all files or use an 'include' pattern.

@michaelyali
Copy link
Member

Ok, I'm checking it. Meanwhile, maybe I should add a build step to the CI, what do you think?

@michaelyali
Copy link
Member

Selection_237

your brancj is working for me. Hm

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

I can only execute yarn s build.util.

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

The structure of this repo does not support the use of npm i github:nestjsx/..., so there is no need to put the output file in the repo.

just build before ci and publish.

but why the tests are depending on js files not ts files?

@michaelyali
Copy link
Member

The structure of this repo does not support the use of npm i github:nestjsx/..., so there is no need to put the output file in the repo.

we use Lerna and publish output files. It doesn't allow publish unstaged files, so we definitely need js files in our repo. But yes, you're probably right - it seems like a wrong decision to test against js files

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

@zMotivat0r solved build problem. config references see https://www.typescriptlang.org/docs/handbook/project-references.html

@michaelyali
Copy link
Member

Ok, cool
add all built js then, push your commit. And please don't forget to rebase to use #100 - use yarn commit

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

@zMotivat0r Why don't use third party libs?
for example lodash

@Diluka
Copy link
Contributor Author

Diluka commented Jun 14, 2019

The structure of this repo does not support the use of npm i github:nestjsx/..., so there is no need to put the output file in the repo.

we use Lerna and publish output files. It doesn't allow publish unstaged files, so we definitely need js files in our repo. But yes, you're probably right - it seems like a wrong decision to test against js files

npm scripts prepare will execute before publish. So, can't you do this?

{
  "scripts":{
    "prepare":"yarn s build"
  }
}

@Diluka
Copy link
Contributor Author

Diluka commented Jun 18, 2019

@zMotivat0r Why did you merge #104 instead of this? And this fix ISO-8601 date string.

@michaelyali
Copy link
Member

@Diluka no worries, we will merge yours too. We need contributors, thats why I'm reviewing all PRs. His PR was good and it fixed that bug, do you agree? I'm not around at the moment, but will try to get to my laptop today evening or tomorrow. Meanwhile could you please coberage?

@Diluka
Copy link
Contributor Author

Diluka commented Jun 19, 2019

@zMotivat0r OK. I'll rebase and refactor with #104 . But when do you remove the compiled files from repo?

@Diluka
fix(compile): set up references to ts projects
730ad02 
build sequence will be in order
@Diluka
chore(test): add more parser tests
3b04643
@Diluka
feat(util): add more assertions
475fc36 
1. `isBoolean` return `true` if val is `boolean` otherwise return `false`
2. `isNumeric` return `true` if val string is a normal number e.g. `-123.456`
3. `isDateString` return `true` if val string is not numeric and can be parsed into a valid date
4. `isDate` return `true` if val is instance of `Date`
5. `isValue` return `true` if val is one of non-empty `string`, `number`, `boolean` or `Date`.
In short, the val can be database value.
6. `hasValue` return `true` if val is `isValue` or non-empty array with each val is `isValue`
@Diluka
feat(parser): add support for ISO-8601 date string
f580ada 
1. refactor #104 to use utils
2. add support for ISO-8601 the date format of `JSON.stringify(new Date())`. Useful for multi-time zone projects.
3. fix some lint error
4. change test. Now filters can receive values as expected.
@Diluka
Copy link
Contributor Author

Diluka commented Jun 19, 2019

emmm, seems like your pretest failed ┑(￣Д ￣)┍ @zMotivat0r

@michaelyali michaelyali merged commit 0a3684e into master Jun 22, 2019
@michaelyali michaelyali deleted the fix/query-parser branch June 27, 2019 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelyali michaelyali Awaiting requested review from michaelyali

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error "Invalid value" when sending a number to the filter in v4.0.0 data types in filters
2 participants
@Diluka @michaelyali