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

Fix #14 by encoding non-alpha characters in attribute keys #37

Closed
wants to merge 1 commit into from

Conversation

cdhowie
Copy link
Collaborator

@cdhowie cdhowie commented Oct 17, 2016

No description provided.

@cludden
Copy link

cludden commented Feb 10, 2017

@cdhowie what's the status on this fix?

@cdhowie
Copy link
Collaborator Author

cdhowie commented Feb 10, 2017

Status is "should work" but OP of #14 hasn't reported any results using it.

@cludden
Copy link

cludden commented Feb 17, 2017

@cdhowie thanks for the follow up. @javatask can you confirm this fixes your issue so we can get this merged. We too have this issue and would like to see it merged as soon as possible.

@cdhowie
Copy link
Collaborator Author

cdhowie commented Mar 3, 2017

This was fixed incorrectly by 7222db6. I am rebasing my patch against v7.1.0 and replacing the changes introduced by this commit.

@cludden
Copy link

cludden commented Apr 2, 2017

@cdhowie will you be updating this PR with your changes?

@cdhowie
Copy link
Collaborator Author

cdhowie commented Apr 6, 2017

@cludden I plan to but have not had time lately to work on it, and the issue doesn't affect us so it's not a priority ATM.

@C45tr0
Copy link
Contributor

C45tr0 commented Jun 20, 2017

What was wrong with the 7222db6 change? Just wondering if it is preference or a situation that is not being handled.

This change looks to be doing the same thing, except it is replacing the characters with a 4 digit equivalent where the 7222db6 is just removing them.

Also, this change looks like it would break expression document paths.

@cdhowie
Copy link
Collaborator Author

cdhowie commented Sep 21, 2017

@C45tr0 The problem arises when you strip out special characters, leaving two attributes with the same cleaned key. Encoding each non-ASCII character as a number prevents this, as every possible attribute key is guaranteed a distinct representation as a cleaned key.

For example, consider the keys abcd and abcdé. If you just strip non-ASCII characters, these two attribute keys have the same cleaned representation (abcd).

C45tr0 pushed a commit to C45tr0/dynogels that referenced this pull request Oct 20, 2017
@C45tr0
Copy link
Contributor

C45tr0 commented Oct 20, 2017

@cdhowie Fair point. I took the liberty to make these changes after the Document paths changes. I will be submitting a PR with that shortly.

@cdhowie
Copy link
Collaborator Author

cdhowie commented Nov 21, 2017

Superseded by #115.

@cdhowie cdhowie closed this Nov 21, 2017
jkav77 pushed a commit that referenced this pull request Dec 3, 2017
C45tr0 pushed a commit to C45tr0/dynogels that referenced this pull request Jul 27, 2018
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.

3 participants