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

ISO date format support #621

Merged
merged 4 commits into from
Jul 17, 2018
Merged

ISO date format support #621

merged 4 commits into from
Jul 17, 2018

Conversation

RichAyotte
Copy link
Contributor

ISO date format based off the code from #166

@@ -1,11 +1,13 @@
import {
GraphQLInt,
Copy link
Contributor Author

@RichAyotte RichAyotte Jul 16, 2018

Choose a reason for hiding this comment

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

Editor cleaned up the off by one space and invalid indentation within this import. Nothing else changed within these braces

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #621 into master will decrease coverage by 2.46%.
The diff coverage is 26.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #621      +/-   ##
=========================================
- Coverage   94.17%   91.7%   -2.47%     
=========================================
  Files          12      13       +1     
  Lines         395     410      +15     
=========================================
+ Hits          372     376       +4     
- Misses         23      34      +11
Impacted Files Coverage Δ
src/typeMapper.js 97.29% <100%> (+0.23%) ⬆️
src/types/dateType.js 8.33% <8.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d682a1b...fc4a819. Read the comment docs.

@mickhansen
Copy link
Owner

This will likely have to be considered a breaking change right?
Remove yarn.lock

@RichAyotte
Copy link
Contributor Author

I'd definitely consider this a breaking change. Sorry about the yarn.lock, new PR coming.

@@ -68,12 +69,15 @@ export function toGraphQL(sequelizeType, sequelizeTypes) {
if (sequelizeType instanceof FLOAT ||
sequelizeType instanceof DOUBLE) return GraphQLFloat;

if (sequelizeType instanceof DATE ||
sequelizeType instanceof DATEONLY) {
Copy link
Owner

Choose a reason for hiding this comment

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

is DATEONLY parsed as a date objecti n Sequelize? Isn't it just a date string?

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 think it depends on the version and the latest is String. See sequelize/sequelize#4858 I'll switch it back String to be on the safe side.

@mickhansen
Copy link
Owner

The new DateType should be exported via src/index

@mickhansen mickhansen merged commit b92521c into mickhansen:master Jul 17, 2018
@RichAyotte
Copy link
Contributor Author

Thanks for the review and quick merge!

@mickhansen
Copy link
Owner

@darkrift
Copy link

The import is bad, the file name is "dateType" not DateType. I installed the module today and it failed because of that.

Error: Cannot find module './types/DateType'
at Function.Module._resolveFilename (module.js:547:15)
at Function.Module._load (module.js:474:25)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object. (/home/rlavoie/work/kerozen/acry/backend/node_modules/graphql-sequelize/lib/index.js:17:13)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object. (/home/rlavoie/work/kerozen/acry/backend/schema/types/root_query_type.js:2:20)
at Module._compile (module.js:652:30)
at Object.Module._extensions..js (module.js:663:10)
at Module.load (module.js:565:32)
at tryModuleLoad (module.js:505:12)
at Function.Module._load (module.js:497:3)
at Module.require (module.js:596:17)
at require (internal/module.js:11:18)
at Object. (/home/rlavoie/work/kerozen/acry/backend/schema/index.js:2:23)
at Module._compile (module.js:652:30)

@RichAyotte
Copy link
Contributor Author

PR is in the queue. #622

@darkrift
Copy link

Hopefully @mickhansen will merge it quickly.

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.

4 participants