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

Add missing has_documentation to MxCommand #167

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

JornVernee
Copy link
Contributor

@JornVernee JornVernee commented Nov 8, 2018

Running mx help <command> throws 'MxCommand' object has no attribute 'has_documentation'. This PR adds that missing function.

This PR:

  • Replaces has_documentation with get_doc, since this simplifies usage.
  • Makes sure the unittest command passes a doc function instead of a doc string (and removed extra newlin in doc string)
  • Makes sure the get_doc function actually returns the documentation string.

@JornVernee JornVernee changed the title Added missing has_documentation to MxCommand Add missing has_documentation to MxCommand Nov 8, 2018
mx_commands.py Outdated Show resolved Hide resolved
@JornVernee
Copy link
Contributor Author

Actually, looking at some of the use cases, like unittest https://github.com/graalvm/mx/blob/master/mx_unittest.py#L351 the documentation never makes it into the doc message because the function right now expects the command's doc string to be a format string.

I think I have a way to fix this.

@graalvmbot
Copy link
Collaborator

Hello JornVernee, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jornvernee -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@dougxc
Copy link
Member

dougxc commented Nov 12, 2018

Hi @JornVernee , can you please ensure all your commits are made with your real email address. This will avoid the complaints by the graalvmbot.

@JornVernee
Copy link
Contributor Author

@dougxc Ah ok. I purposely stopped adding my emails to the commits since I was getting a lot of recruitment spam.

I will add the email with the next commit.

@dougxc
Copy link
Member

dougxc commented Nov 12, 2018

Thanks and sorry to hear about the spam.

You'll also need to redo the commit (rebase) that is using the github email address.

@JornVernee
Copy link
Contributor Author

Not sure why Travis is choking, I can not reproduce that error on my machine. Is something going on with the CI setup?

mx.py Outdated Show resolved Hide resolved
@gilles-duboscq
Copy link
Member

gilles-duboscq commented Nov 13, 2018

Not sure why Travis is choking, I can not reproduce that error on my machine. Is something going on with the CI setup?

Seems to be an issue with the JDT (eclipse compiler/ECJ) JAR which comes from there:

mx/.travis.yml

Line 18 in 9c00c2e

wget http://www.eclipse.org/downloads/download.php?file=/eclipse/downloads/drops4/R-4.5.2-201602121500/ecj-4.5.2.jar -O $JDT

At least locally this URL works fine. I'm not sure what's going on. Maybe the download got corrupted?

@JornVernee
Copy link
Contributor Author

JornVernee commented Nov 13, 2018

@gilles-duboscq I went to the URL and there seems to be no re-direct to https:// either (I had that problem a few days ago on another project)

But using https:// also seems to work, maybe you could try switching to that?

changed to get_doc function, since that makes use simpler

commit right version this time :-)

Changed implementation to now return the doc correctly, test with `mx help unittest`

typo fix

Add command function doc to help message (accidentally removed it earlier)

- Only print <no documentation> if there is not usage_msg, doc string or doc function.
- Changed implementation slightly to make sure that doc is formatted correctly when some of these are missing
@JornVernee
Copy link
Contributor Author

@gilles-duboscq Well, I implemented your changes and the travis check is passing as well now. My other PR had the same problem: #172 Is there any way you could re-run the check?

Thanks

@gilles-duboscq
Copy link
Member

Looks good now, thank you!

Regarding #172, maybe once this is integrated, you can rebase it? For some reason it seems to also contain the changes from #164 which has already been integrated so maybe a rebase is anyway a good idea.

@gilles-duboscq
Copy link
Member

Also this is changing a line with print so it will definitely conflict with #172.

@JornVernee
Copy link
Contributor Author

JornVernee commented Nov 13, 2018

OK, I'll do that after this is integrated then. Then I can make sure there's no other conflicts or weird diffs with mx/master at that point as well.

@dougxc dougxc merged commit c8cf1bb into graalvm:master Nov 13, 2018
@JornVernee JornVernee deleted the has_doc branch November 14, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants