Warn about solidity in assets directory

[SamWilsn]

Originally from #55

In the Specification section, suggest to replace all links to solidity files to inline versions.

[JEAlfonsoP]

I will work on it.

[MicahZoltu]

All external links, or do you think we should suggest replacing links like ../assets/eip-####/file.sol as well?

[Pandapip1]

I'd prefer it if the bot gave an option to replace all links to solidity code with code blocks (à la #75).

[MicahZoltu]

Currently, we tell people who link to external code that they should either inline the code, or add it to ../assets/eip-####/file.sol. Are you suggesting that we instead tell them they should inline it and not give them the option of adding it to ../assets/eip-####/file.sol?

If so, I'm curious about the reasoning for such a change? It isn't clear to me what benefit we gain by disallowing code in assets.

[JEAlfonsoP]

I am confused, original action was just to warn or suggest to replace all links to solidity files to inline versions. !!
Please could we agree on the action for this issue so I can propose a possible solution (i.e. warning the author about it before merge..)

[Pandapip1]

It isn't clear to me what benefit we gain by disallowing code in assets.

I agree - if anything, this makes EIPs more verbose at the expense of readability. EIPs should be for reading, assets should be for examining.

[JEAlfonsoP]

If I understood this, action would be to warn author about replace links to solidity files to inline versions.
If this is the case a PR will be created right away.

[MicahZoltu]

The bot would parse the whole EIP and look for any external links. If it finds one that ends in .sol, it will comment on that line and recommend the user include the code inline in the EIP, rather than externally.

[JEAlfonsoP]

Question:
Do we want the bot to warn while parse /[\S]/[\S]*.sol and recommend inline coding for each appearance or do we actually want to modify PR body and "comment" in it for each appearance ?

/[\S]/[\S]*.sol

[MicahZoltu]

I think /[\S]/[\S]*.sol is too restrictive as it would catch ../assets/eip-####/foo.sol. I think we want something like http\S+.sol.

When we find a match, it should at least fail CI. Second best option would be to let the user know in a comment what the problem is. Third best option would be to comment directly on the line with the external link. Fourth best option would be to collect this error up along with any others and submit a PR review with the feedback.

I recommend we start with the first option (just fail CI with a console.log), and then we can iterate/improve on it later.

[Pandapip1]

There are legitimate use-cases for solidity in the assets directory, however. I'd prefer if it just warned the user.

[MicahZoltu]

Our policy on external links is quite strict at the moment, so I have a strong preference for failure over warning. Why do you think we should only warn?

[Pandapip1]

I was talking about internal links to files in the assets directory. I agree external links should fail PR unless an editor approves it.

EDIT: This issue is about solidity in the assets directory, not about external links. The issue you are thinking of is #73

[MicahZoltu]

Hmm, my interpretation of this issue is that it is referring to external links to solidity files, which is incredibly common for EIP authors to include.

[MicahZoltu]

I don't think we should do anything at all if someone has a correctly formed internal link to a .sol file. It is up to the individual authors whether they think inline is better/worse than ../assets.

[SamWilsn]

So to summarize:

• Relative (aka internal) links, which may take the form of [Something.sol](../assets/eip-XXXX/Something.sol), do not generate a warning/error/comment. They also do not block the merge.

• Absolute (aka external) links, which may take the form of [Something.sol](http://example.com/Something.sol), do generate a warning/error/comment directing the author to either inline the file or place it in the assets directory. They also do block the merge.

[Pandapip1]

Fixed in eipw.