Skip to content

Fix for #193:deprecate XMLEventReader #199

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

Closed
wants to merge 1 commit into from
Closed

Fix for #193:deprecate XMLEventReader #199

wants to merge 1 commit into from

Conversation

mghildiy
Copy link
Contributor

@mghildiy mghildiy commented Mar 18, 2018

class XMLEventReader in package pull is deprecated(since 1.1.1)

@ashawley
Copy link
Member

Thanks for this.

Doesn't look like your commit is assigned to your GitHub account. Did you configure your e-mail in git? See https://help.github.com/articles/setting-your-commit-email-address-in-git/

After fixing git, you should also update the commit you made here with a command like this:

$ git commit --amend --author 'manish <your@email>'
$ git push -f origin master

It's also good to use a Git workflow with branches, see:

https://help.github.com/articles/github-flow/

@ashawley
Copy link
Member

No description provided.

Descriptions in pull requests are usually helpful for reviewiers. For this PR, one thing worth mentioning is what the deprecation message is for the compiler:

class XMLEventReader in package pull is deprecated (since 1.1.1):
XMLEventReader is an old class,potentially not up to expected standards
of quality.Use javax.xml.stream.XMLEventReader instead.

Maybe the deprecation message should just be:

class XMLEventReader in package pull is deprecated (since 1.1.1): Consider
javax.xml.stream.XMLEventReader instead.

@ashawley ashawley added this to the 1.2.0 milestone Apr 7, 2018
@SethTisue
Copy link
Member

@mghildiy still interested in pursuing this...?

@mghildiy
Copy link
Contributor Author

Review comment by @ashawley has been implemented, and committed. Or am I still missing something here?

@SethTisue SethTisue requested a review from ashawley April 16, 2018 11:08
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

just realized scala.xml.pull.XMLEvent should be deprecated too, shouldn't it?

@@ -24,6 +24,7 @@ import scala.xml.parsing.{ ExternalSources, MarkupHandler, MarkupParser }
* @author Burak Emir
* @author Paul Phillips
*/
@deprecated("class XMLEventReader in package pull is deprecated(since 1.1.1):Consider javax.xml.stream.XMLEventReader instead.", "1.1.1")
Copy link
Member

Choose a reason for hiding this comment

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

Or am I still missing something here?

The deprecated tag message should just be "Consider javax.xml.stream.XMLEventReader instead.". Currently, the message is duplicated by the compiler:

class XMLEventReader in package pull is deprecated (since 1.1.1): class XMLEventReader 
in package pull is deprecated(since 1.1.1):Consider javax.xml.stream.XMLEventReader instead.

Copy link
Member

@ashawley ashawley left a comment

Choose a reason for hiding this comment

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

just realized scala.xml.pull.XMLEvent should be deprecated too, shouldn't it?

Yes, it probably should be deprecated, as well.

Copy link
Member

@ashawley ashawley left a comment

Choose a reason for hiding this comment

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

LGTM

@ashawley
Copy link
Member

ashawley commented Apr 16, 2018

I'd suggest rebasing, but your branch name is called master. I guess you could try git pull --rebase upstream master and then git push --force-with-lease origin master, but at your own risk. If not, I can do the rebase to avoid the merge bubble.

@SethTisue
Copy link
Member

I don't think there's a problem, the contents of the PR seem fine.

@ashawley ashawley modified the milestones: 1.2.0, 1.1.1 Apr 16, 2018
ashawley added a commit that referenced this pull request Apr 16, 2018
Fix for #193:deprecate XMLEventReader
@ashawley
Copy link
Member

Merged in 1b951af. Thanks for this contribution.

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