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

use ParserContext and add more element types in HTMLSchema (default) #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manish211
Copy link

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@@ -153,7 +153,7 @@ private static void process(final String src, final OutputStream os) throws IOEx
} else {
r = new Parser();
}
theSchema = new HTMLSchema(true);
theSchema = new HTMLSchema(new ParserContext(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

get the Context from the parser instance above.

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines 320 to 328
/**
* Set the parser context.
* @param parserContext the parser context
*/
public void setParserContext(final ParserContext parserContext) {
this.parserContext = parserContext;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this, this is dangerous

Copy link
Author

@manish211 manish211 Nov 26, 2019

Choose a reason for hiding this comment

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

done. Now constructor will have it instead of setter.

@@ -345,7 +376,8 @@ public void setFeature(final String name, final boolean value) throws SAXNotReco
} else if (name.equals(CDATA_ELEMENTS_FEATURE)) {
cdataElements = value;
} else if (name.equals(STRING_INTERNING_FEATURE)) {
useIntern = value;
parserContext.setUseIntern(value);
// theSchema.setUseIntern(useIntern);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link
Author

Choose a reason for hiding this comment

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

done

* Get the useIntern flag value.
* @return useIntern flag value
*/
public boolean getUseIntern() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

removed

* Set the use intern flag.
* @param useIntern whether to use string intern or not
*/
public void setUseIntern(final boolean useIntern) {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Author

@manish211 manish211 Nov 25, 2019

Choose a reason for hiding this comment

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

changed the class to be not final and made the methods final.

* @param input the input string.
* @return reference to the string.
*/
public String getReference(final String input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Author

Choose a reason for hiding this comment

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

done

/**
* Clear the state.
*/
public void clear() {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Author

Choose a reason for hiding this comment

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

done

*/
protected void setUseIntern(final boolean useIntern) {
this.useIntern = useIntern;
public ParserContext getParserContext() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be private

Copy link
Contributor

Choose a reason for hiding this comment

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

or removed

Copy link
Author

Choose a reason for hiding this comment

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

done

* Set the parser context.
* @param parserContext the context of the parser
*/
public void setParserContext(final ParserContext parserContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

private

Copy link
Author

Choose a reason for hiding this comment

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

made protected. This method is needed in Parser class to set it on the schema object.

/**
* Clear the state.
*/
public void clear() {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@lafa lafa left a comment

Choose a reason for hiding this comment

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

check the comments

@@ -2192,6 +2192,7 @@
<element name='a' type='mixed'>
<memberOf group='M_INLINE'/>
<contains group='M_NOLINK'/>
<contains group='M_BLOCK'/>
Copy link
Contributor

Choose a reason for hiding this comment

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

please add UTs for these 3 changes please

Copy link
Contributor

Choose a reason for hiding this comment

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

I need the comments to explain why are these changes here

Copy link
Author

Choose a reason for hiding this comment

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

done

@ashutoshvsingh
Copy link
Contributor

👍

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

3 participants