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 lint preventing EIP references in backticks #90

Closed
SamWilsn opened this issue Mar 11, 2024 · 11 comments · Fixed by #93
Closed

Add lint preventing EIP references in backticks #90

SamWilsn opened this issue Mar 11, 2024 · 11 comments · Fixed by #93
Assignees
Labels
good first issue Good for newcomers ready to implement Feature has reached rough consensus among editors and is ready to be implemented

Comments

@SamWilsn
Copy link
Contributor

Many authors attempt to circumvent the EIP link requirements by putting EIP-XXXX in backticks (eg. `EIP-1234`.) This is bad form, and should be rejected.

@SamWilsn SamWilsn added good first issue Good for newcomers ready to implement Feature has reached rough consensus among editors and is ready to be implemented labels Mar 11, 2024
@VictoriaAde
Copy link
Contributor

@SamWilsn I'd like to work on this.
Can I be assigned?

@SamWilsn
Copy link
Contributor Author

Done! Be aware that we need to be careful to allow things like ERC20 or IERC7777 because they're interface names, but prevent EIP-7777 because that isn't valid code.

@VictoriaAde
Copy link
Contributor

Noted!

@Hari-Bombon
Copy link

@SamWilsn I would love to work on this repo. Could you please assign me?

@SamWilsn
Copy link
Contributor Author

@Hari-Bombon we should give the user above a chance to work on this, since she asked first. #11 or #5 might be good candidates to try instead?

@VictoriaAde
Copy link
Contributor

Hi @SamWilsn, I have added the lint rule but my test keeps failing.
What do you think I'm doing wrong?

image

@SamWilsn
Copy link
Contributor Author

@VictoriaAde I'd need to see a bit more of your code (do you have a branch somewhere?) but if I had to guess, it looks like your src variable isn't a correctly formatted EIP document. It's missing the --- somewhere.

@VictoriaAde
Copy link
Contributor

@SamWilsn I looked at what you said and now if I run cargo run or test, it will giving the below message.
<SOURCE>... I can't seem to figure out what the issue is.

image

@SamWilsn
Copy link
Contributor Author

Right, forgot about that. The project is divided into a few crates (aka packages):

  • eipw is at the root of the repository, and provides a command-line interface to the tool.
  • eipw-preamble is a library that provides functions for parsing the preamble of EIPs (the stuff at the beginning of proposals between --- lines.)
  • eipw-lint is where the majority of the interesting code lives, including the lints themselves.
  • eipw-lint-js is the glue that connects to JavaScript, and will likely be uninteresting for this issue.

To run the tests for lints, you'll need to change directory into eipw-lint, and run the tests there:

cd eipw-lint
cargo test

Alternatively, you can run all the tests from the root directory:

cargo test --all

Regarding the <SOURCES>... message, the tool is asking for the file you'd like to check. For example:

cargo run -- ~/EIPs/EIPS/eip-3074.md

@VictoriaAde
Copy link
Contributor

Oh my, thank you so much.
I will try these out.

@VictoriaAde
Copy link
Contributor

@SamWilsn, I made a PR, please check it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers ready to implement Feature has reached rough consensus among editors and is ready to be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants