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

editorconfig/editorconfig#429 Remove section name, key and value length limits #41

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

goodhoko
Copy link
Contributor

  • Remove the upper limit for section name, key and value length.
  • Permit implementations to define their own upper limits.
  • Add minimum supported lengths each implementation must support.

@ppalaga
Copy link
Contributor

ppalaga commented May 21, 2020

@goodhoko I do not see a problem when looking at the changes. But to get real certainty I would like to see the changes necessary for ec4j to pass the new tests. Do you happen to have the changes already? Otherwise I can try.

@goodhoko goodhoko changed the title editorconfig/editorconfig#429 Remove sectio name, key and value length limits editorconfig/editorconfig#429 Remove section name, key and value length limits May 21, 2020
@goodhoko goodhoko force-pushed the feature/remove-length-limits branch from f6950b4 to a433049 Compare May 21, 2020 11:18
@goodhoko
Copy link
Contributor Author

@ppalaga Validating the tests against an actual implementation would certainly be awesome.

I do not have anything ready yet. So feel free to do that!

@ppalaga
Copy link
Contributor

ppalaga commented May 21, 2020

@goodhoko I hope I'll be able to do it in a couple of days.

parser/limits.in Outdated Show resolved Hide resolved
Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

ec4j is passing with these changes after I removed the limits ec4j/ec4j#111

@cxw42
Copy link
Member

cxw42 commented May 26, 2020

ec-vim passes on my local machine after removing limits: editorconfig/editorconfig-vim#148

Copy link
Member

@cxw42 cxw42 left a comment

Choose a reason for hiding this comment

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

One question

parser/limits.in Outdated
[{test4,test00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004}]
key=value
; minimum supported section name length of 1024 characters
[{test3,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa}]
Copy link
Member

Choose a reason for hiding this comment

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

There are only 1014 as in this string, and the total length inside the brackets is 1022. If I read https://github.com/editorconfig/specification/pull/21/files#diff-db4b047b30ffef7855b12b527860ac2cR134 correctly, there should be 1024 characters inside the brackets here, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. The old limit was tied to the length of the glob without brackets - at least that's how it was implemented in ec4j. I wonder if we should make this more explicit in the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should make this more explicit in the spec?

Reading again, I think it is clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch indeed! I based the tests on the existing ones, but confused test3 and test4. 🤦

I fixed this and also added note in the comment in the limits.in

; minimum supported section name length of 1024 characters (excluding [] brackets)

Reading again, I think it is clear enough.

Agree. It directly follows from

Section Name: the string between the beginning [ and the ending ].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cxw42 can you take a look at this, please? I believe it should be all good now.

I also run the 'limitless branch' of ec4j against this branch and everything passed. (thanks @ppalaga )

@ppalaga
Copy link
Contributor

ppalaga commented Jun 18, 2020

@cxw42 isn't this good to merge?

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

😸 LGTM

@cxw42
Copy link
Member

cxw42 commented Jul 19, 2020

@ppalaga I think we have enough successes that the spec change could now be put to a vote. I see there are some merge conflicts to be resolved @goodhoko

…th limits

- Remove the upper limit for section name, key and value length.
- Permit implementations to define their own upper limits.
- Add minimum supported lengths each implementation must support.
@goodhoko goodhoko force-pushed the feature/remove-length-limits branch from a4fcbb3 to cf3a942 Compare July 21, 2020 12:41
@goodhoko
Copy link
Contributor Author

@cxw42 Rebased.

@xuhdev
Copy link
Member

xuhdev commented Dec 2, 2020

Should be be ready to be merged yet?

@cxw42
Copy link
Member

cxw42 commented Dec 4, 2020

@xuhdev pending editorconfig/specification#21, but other than that, I think it's probably fine. The only thing is that I have not had a chance to investigate the Windows test failures on editorconfig-vim (my apologies!), but I agree we can proceed anyway.

@xuhdev
Copy link
Member

xuhdev commented Dec 5, 2020

Sorry for the delay -- sometimes I miss notification or forget things. I've started a polling: editorconfig/editorconfig-vote#13

Feel free to ping me next time -- I'm never tired of reminders!

@goodhoko
Copy link
Contributor Author

@xuhdev since editorconfig/specification#21 is merged, this should be merged as well to keep the spec and tests in sync. .)

@xuhdev
Copy link
Member

xuhdev commented Dec 28, 2020

Thanks for your contributions! Sorry for having missed this PR earlier.

ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
ppalaga added a commit to ppalaga/ec4j that referenced this pull request Dec 28, 2020
cxw42 added a commit to cxw42/editorconfig-core-c that referenced this pull request Oct 30, 2022
Increase the minimum key and value lengths to pass the tests added in
<editorconfig/editorconfig-core-test#41>.
cxw42 added a commit to cxw42/editorconfig-vim that referenced this pull request Oct 31, 2022
Increase the minimum key and value lengths to pass the tests added in
<editorconfig/editorconfig-core-test#41>.
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.

None yet

5 participants