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

WIP: Web.JSON improve to v8 #695

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

WIP: Web.JSON improve to v8 #695

wants to merge 9 commits into from

Conversation

Milly
Copy link
Contributor

@Milly Milly commented Nov 11, 2019

Web.JSON is too old.
Update to vim 8.

...But since json_encode() already exists, is this module unnecessary?

This PR includes these new featues.

  • 'allow_nan' setting.
  • 'ensure_ascii' setting.
  • 'from_encoding' setting.

@Milly Milly changed the title Web.json v8 Web.JSON improve to v8 Nov 11, 2019
@ujihisa ujihisa requested review from lambdalisue and mattn November 11, 2019 19:56
@lambdalisue
Copy link
Member

From my point of view, at least this module should keep backward compatibility while

  1. People should use json_encode() instead as you said
  2. That's why there is no positive reason to improve this module
  3. Code which uses this module uses this module may because of some reason

@Milly
Copy link
Contributor Author

Milly commented Nov 12, 2019

  1. People should use json_encode() instead as you said

Vital.vim is currently (basically) support vim 8.0 or later.
So should this module be deprecated?
Write to document "deprecated" and "should use json_encode()"?

  1. That's why there is no positive reason to improve this module

I'm thinking of an implementation that makes the JSON module more flexible like Python's JSON Library.
json_encode() is not-strict and not-flexible.
Should this be a separate module? (eg. Data.JSON or ...)

  1. Code which uses this module uses this module may because of some reason

Agree

One thing, These are purely bug fixes.

  • Convert control chars. (Encoded invalid JSON)
  • Convert UTF-16 surrogate pairs. (Can not decode valid JSON)

@lambdalisue
Copy link
Member

lambdalisue commented Nov 13, 2019

So should this module be deprecated?
Write to document "deprecated" and "should use json_encode()"?

Yeah, I think so but it's just my opinion. Actually, I'm more welcome to remove this module rather than improve/fix because of existnace of json_encode().

I'm thinking of an implementation that makes the JSON module more flexible like Python's JSON Library.
json_encode() is not-strict and not-flexible.
Should this be a separate module?

Got it. Well, then I think it's better to

  1. Use different namespace as you said (I'm OK with Data.JSON)
  2. Remove any codes for backward compatibilities and explain the benefits of using Data.JSON over json_encode() or whatever in help
  3. Apply bug fixes to Web.JSON (Optional, if you desire)

Copy link
Contributor

@tsuyoshicho tsuyoshicho left a comment

Choose a reason for hiding this comment

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

I Think Changes OK.

But. If you review result read and think to need changing.

  • Vim.Type use or not
  • add Changes

項目がのこったままになってる面があったので、レビューがんばってみました。
(ありすえさん指摘の点をどうするかは別として、内容自体は問題ないと思います。(自信は微妙ですが))

そのまますすめるにしても、指摘点についてどうするか確認の意味もあり、 Request changes で返しています。

autoload/vital/__vital__/Web/JSON.vim Show resolved Hide resolved
corresponding javascript tokens (e.g. 'true').
Otherwise 1 or 0 are used to represent these.
The default value is 0.
'allow_nan'
Copy link
Contributor

Choose a reason for hiding this comment

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

This point is incompatible changing. need appending item to Changes.

Please Changes appending to commit after all changes are done, and before PR merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the point you say Add 'allow_nan' or Remove 'use_token'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

I think, Adding phrase to Changes for example Improve and *use_token* remove, and option renewal to support *allow_nan* .

@Milly Milly mentioned this pull request May 21, 2020
@Milly
Copy link
Contributor Author

Milly commented May 21, 2020

I'm created #743 that part of bug fixes of this PR.

@Milly Milly changed the title Web.JSON improve to v8 WIP: Web.JSON improve to v8 Jun 11, 2020
@mattn
Copy link
Member

mattn commented Jul 28, 2020

Closed by #743 ?

@Milly
Copy link
Contributor Author

Milly commented Jul 28, 2020

@mattn

Closed by #743 ?

It's includes only bug fixes that is part of this.

I'm rebased, skip these commits and force pushed.

Copy link
Member

@lambdalisue lambdalisue left a comment

Choose a reason for hiding this comment

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

Sorry for late. I think it would be better to add a new one like Data.JSON or whatever and mark this module obsolete but while the code seems OK, approved 👍 I don't care if this PR is merged as-is if others do not care about my point of view.

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