Add pret style option

[anmart]

I've added a --pret-style arg that changes output a bit to match disassemblies written for pret. It includes the following:

• All function labels are Func_[Global Addr] instead of [type]_[bank]_[local addr]
  • Function labels also have comments of the form ; [global addr] ([bank]:[local addr])
• one newline after conditionless returns and jumps and no newlines otherwise
• ; fallthrough comment when a label begins without a conditionless return or jump

I'm not married to --pret-style as a name, in fact I quite dislike it. I'm just not creative enough for something else

[ISSOtm]

Wouldn't it be better to split that into several flags? For example, one could want fallthrough comments but keep the rest

[mattcurrie]

Thanks @anmart, this is looking good!

I agree with @ISSOtm, it would be nice if these were available as separate flags.

Perhaps there could be a --theme (?) arg which could take a value of pret to set all the required flags for pret? Later on there could be support for other themes for other disassembly styles.

[ISSOtm]

I'd be for --style instead of --theme. Makes more sense for a coding style.
What would the individual flags be, though?

Here's an idea:
--style param=value (such as --style fallthrough=yes)
--preset name (such as --preset pret)

[anmart]

Thank you both for the feedback! Naming is by far my least favorite part about programming so this is helpful.

I'd be for --style instead of --theme.

I do like style, but internally that would be referred to as style['style'] which I think could be a bit confusing. I suppose the flag could be one thing while the dict entry could be another, but that could be even worse in terms of confusion.

I think the param=value system would be good to reduce the amount of main level flags. I'm envisioning something where

• preset sets flag values initially
• param=values are checked and overwrites flag values set by preset
• if a param or value is invalid, immediately abort, possibly displaying the help message
• all flags in their own dict? or in the main dict with a style_ prefix

I'm not sure how exactly they could be documented though. If they were all crammed into the --style flag it could get pretty messy

[mattcurrie]

Yes, --preset sounds fine instead of --theme.

The --style param=name idea sounds good, as long its possible to coerce argparse (or something else?) - to support that style of argument. We could have a separate argument something like --style-help which displays the full list of style flags to tidy up the main help.

Prefer to keep the separate styles dict for the style flags.

Just to update your proposed flow:

• initially set default style flag values in styles dict (assuming argparse won't be able to supply the default values for the style flags anymore).
• preset overwrites any style flag values it needs to
• param=values are checked and overwrite style flag values
• if a param or value is invalid, immediately abort, possibly displaying the help message

[ISSOtm]

The --style param=name idea sounds good, as long its possible to coerce argparse (or something else?) - to support that style of argument.

If it can't parse that itself, it can just be treated as a raw string and parsed by the program itself.

We could have a separate argument something like --style-help which displays the full list of style flags to tidy up the main help.

Seconded, as with the proposed flow. That said, should all --style override --presets, or should everything be applied in the order it's been parsed?
I'd prefer the latter as the former would cause --presets to override --styles

[anmart]

I'd prefer the latter as the former would cause --presets to override --styles

I'm afraid I don't understand what you're saying. The former would specifically be all --styles overriding all --presets wouldn't it?

it can just be treated as a raw string and parsed by the program itself.

I agree, this was my plan since I don't believe argparse has any direct support for this sort of format.

Other than that I think this is good to go. I'll start working and will ask for feedback later. Thanks for the help!

[ISSOtm]

The former would specifically be all --styles overriding all --presets wouldn't it?

Yep, whereas the first would have any --style to the left of a --preset would be overridden.

[anmart]

Yep, whereas the first would have any --style to the left of a --preset would be overridden.

But the First/Former one was "should all --style override --presets" while the second/latter one was "should everything be applied in the order it's been parsed" wasn't it?

so the latter would be the one causing presets to overwrite styles while the former would be the one that would have styles always overwrite presets?

Sorry, I'm getting a bit confused from this

[ISSOtm]

No matter how it's implemented, the gist of it was that I'd rather see all --style override all --preset no matter the position. :P

[anmart]

I figured out where my confusion was, sorry about that!

I'm a bit stuck on some organization matters though -- Most every flag currently in mgbdis is already a style flag. Should I grandfather those in and say they're important enough to warrant their own --name style flags? Or is the intention to also add these to the flag=value and preset system?

If the latter, certain more important flags could have both a flag=value and a --flag setting method. Possibly all of the already implemented ones to preserve compatibility.

[mattcurrie]

Looks like everything except --output-dir, --overwrite and --debug are style flags. All the others can be moved to the flag=value style arguments.

I'm not sure about whether grandfathering the old style flag arguments is necessary - especially if the goal is to clean up the arguments a bit. The only project I know of that is making use of them is the LADX disassembly and we could easily add a preset for them too. In addition, this release could bump the version to 2.0 to indicate a breaking change.

[anmart]

I think fe43fd2 shows that I wasn't prepared to do much more complex python than the first few commits lol.

I tried to do something similar to what argparse already does to keep it concise and easy to update. I think there's a lot that could be fixed but I'm not sure how best to do that.

The return "ERROR" is temporary and it'll instead print out the help message.

[anmart]

With the latest commit I've made it so you can actually pass style options in through arg parse (not that they work). However, since it uses nargs='+', it causes issue with the rom_path arg. If --style comes immediately before the rom_path, it'll assume the rom is another style option. I'm not sure how best to express this to the user, or if there's anything that can be done to change this behavior.

[mattcurrie]

Insetad of nargs='+', could use action='append' to allow multiple separate --style options to be provided, or otherwise let style accept a comma separated list of style arguments?