search

dseight / vscode-disasexpl

Disassembly Explorer for VS Code
MIT License
48 stars 11 forks source link

Removed some weird code to get the document associated with an URI #15

Closed Delta-dev-99 closed 2 years ago

Delta-dev-99 commented 3 years ago

Removed some weird code to get the document associated with an URI. Also renamed a couple variables to make easier to read the source. Hope this helps.

dseight commented 2 years ago

Thank you for your contribution!

Could you, please, fix variables naming to use lowerCamelCase so they will follow the project's coding style?

Delta-dev-99 commented 2 years ago

Sure, I will

Delta-dev-99 commented 2 years ago

Variable names should be OK in a minute. Im finishing it. I'm happy to contribute!

dseight commented 2 years ago

Tried to compile and encountered some import errors. @Delta-dev-99 , have you tested it?

Delta-dev-99 commented 2 years ago

Let me check that out

Delta-dev-99 commented 2 years ago

Did the first version (the one without the lowerCamelCase naming) compile? I think I tested it, but it was a long time ago. Now I don't even have the same system.

Delta-dev-99 commented 2 years ago

Actually I found the bug. I forgot to rename the function name in the import.

dseight commented 2 years ago

Did the first version (the one without the lowerCamelCase naming) compile?

I've just checked it out — nope, it doesn't compile. Neither does the current one, because of the following error:

src/provider.ts:43:35 - error TS2304: Cannot find name 'TextDocument'.

43 export function getAsmUri(source: TextDocument): Uri {
                                     ~~~~~~~~~~~~

It's wise to test the code before pushing it. :slightly_smiling_face:

To prevent such things from happening in the future, I've set up a Github Actions. Now it should be a little bit more transparent whether PR is buildable or not.

Delta-dev-99 commented 2 years ago

Hello! I don't have a compiler available. I'm actually correcting this code from my cell phone (I love coding). I really hope it works now. This is embarrassing... I need your approval to run the github actions workflow. It would be a game changer to be able to at least check the code compiles.

dseight commented 2 years ago

Just for future PRs: I would not recommend changing variable names that are unrelated to PR functionality. But other than that, PR looks good now.