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

Improved test, fix for multiple codeblocks, code quality and comments #1

Merged
merged 4 commits into from
Oct 14, 2021

Conversation

sezanzeb
Copy link
Contributor

@sezanzeb sezanzeb commented Oct 12, 2021

```
a
```

```
b
```

resulted in

a
a
b
  • renamed some variables and added comments to them to make understanding the code easier
  • more consistency with if statements: space after if everywhere, no one-liner ifs
  • consistency with spaces around assignment operators
  • the switch only had one case and the default. Using if/else now instead
  • The test compares two files now if they are equal and shows the difference instead of just counting their lines. I would recommend using mocha instead of bash for testing in future projects.

@ubunatic
Copy link
Owner

LGTM. But the GTK demos are now broken (Just change the MD file path to tests/test.md; please do not reformat them).

Also I never used mocha before and try to avoid any package and dev dependencies if possible.
Thx for the idea with the diff, Dunno why I did not do that myself.
Mocha would pull in a lot.

Regarding code formatting, I can live with the proposed "traditional" curly-heavy, JS style in the package's main JS file. Not sure if this makes code more readable. Some yes, some no. Loosing the "tables" and their block comments is really unfortunate. Code formatters in 2021 are still so dumb.

Do you think there is any useful autoformatter that is able to auto-detect table-like structures such as the replacement expressions and tableize the JS code nicely? This would be much more readable.

@ubunatic
Copy link
Owner

And of course. Thanks for looking into this project and the first PR!
I though I am the only guy on the planet trying to do some stuff with GTK and JS outside of the Gnome Shell ecosystem ;)

@sezanzeb
Copy link
Contributor Author

sezanzeb commented Oct 13, 2021

Mocha would pull in a lot.

I think devDependencies are only downloaded when installing from a git clone, i.e. running npm install from within the projects root. When installing via npm install ... I don't think they are downloaded.

Not sure if this makes code more readable

I guess you are referring to the change to the one-liner if statements? My colleague who has really been into clean code for many years says consistency is the most important aspect of every style. So every if statement is formatted the exact same way, no matter the length. And I really agree with him

Loosing the "tables" and their block comments

Do you mean for example m2p_styles? I can see why one would prefer to align each element in that object underneath each other, and I think I also used to do that.

One argument against it I could think of would be:

{ a: ".....................................", b: "" },
{ a: "", b: "....................................." }

formatting this as a table could potentially break the max line length rule. So to obtain consistency, every object has to be formatted without using extra whitespaces.

Second argument would be, that if "a" in the first line changes length, I have to add whitespaces to every other line. Which is also why I really dislike the indentation some python developers use:

def aaaaaaaaaaaa(b, c):
    ...

aaaaaaaaaaaa(1234,
             5678)

which can get REALLY out of hands quickly if function calls become more complex. Unfortunately I haven't saved the worst example I have ever seen as a bookmark... And if any of the potentially nested calls (which shouldn't be nested in the first place) changes the function name, all of the whitespaces after that are misaligned. I'm really glad the "black" python formatter doesn't do that.

Do you think there is any useful autoformatter that is able to auto-detect table-like structures such as the replacement expressions and tableize the JS code nicely? This would be much more readable.

No. I have only used eslint so far with a modified set of the airbnb rules, which supports a --fix command line option, so I can't really make a recommendation for this since I don't know any alternative.

I though I am the only guy on the planet trying to do some stuff with GTK and JS outside of the Gnome Shell ecosystem ;)

I was thinking of generating offline docs from some markdown files for a python GTK app, which are currently only visible on github. They would even be translatable with gettext. We decided that linking the markdown files would be fine as well though. I can still see that this would be a very valid use case. To make installation for everyone easy without requiring node.js as a dependency one option would have been a pre-commit hook maybe.

Thanks for looking into this project and the first PR!

You are welcome :)

@sezanzeb
Copy link
Contributor Author

sezanzeb commented Oct 13, 2021

If you would like to keep the table-like structure of m2p_styles and such I'll revert that. That was just my 2 cents on formatting, no strong feelings on my side.

@ubunatic
Copy link
Owner

If you would like to keep the table-like structure of m2p_styles and such I'll revert that. That was just my 2 cents on formatting, no strong feelings on my side.

Nah, it is OK. readable enough. I will just wait 20 more years until the formatters become smarter.

@ubunatic
Copy link
Owner

Regarding node.js-less installation, maybe create an issue and let's discuss it there. I did not get the pre-commit-hook + installation story.

@ubunatic ubunatic merged commit d437d3c into ubunatic:main Oct 14, 2021
@sezanzeb
Copy link
Contributor Author

Regarding node.js-less installation, maybe create an issue and let's discuss it there. I did not get the pre-commit-hook + installation story.

The idea was that when making commits, md2pango would convert the markdown files to pango automatically, so that when installing the package from the latest git source the user interface would be up to date.

Since it's a python tool, adding node.js as a dependency wouldn't seem appropriate for the users.

Anyway, don't worry about that, this is a question of how to integrate md2pango in other projects and there are many ways to do it. Thanks for merging

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.

None yet

2 participants