Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generic: Add first attempt at pgdscan plugin #1321
base: develop
Are you sure you want to change the base?
Generic: Add first attempt at pgdscan plugin #1321
Changes from 1 commit
e4072c7
6195eeb
18ecbc7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also somewhat hacky, buy you could presumably just copy the last character
_number_of_pointers_per_page - 1
number of times. Still kinda hacky (and still relies on the format being a single letter, but it's likely and allows for both alignment and no alignment value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that's a nice idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep an eye out for enhancements to the config system that should allow configs to be more reusable across plugins that have different requirements (
TranslationLayerRequirement
rather thanModuleRequirement
, for example).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't yet have a suitable way of guaranteeing this is the lower layer (and this may not work if the lower layer has been swapped out, etc), but until we have something better this is ok. Be nice to flag it with a FIXME or a TODO, just so we can find it again in the future...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - it's something that does pop up a fair bit. I couldn't see an issue tracking it. Do you think it's worthwhile making one? (e.g. so that it's "TODO: Re issue XXXX update to a more suitable way of guaranteeing this is the lower layer")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we never explicitly made one, but it might be good to see how many other issues might depend on it? Happy for you to spin that up, or shout and I can do it too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you just use a prefix of
primary
rather thanIntelLayer
, it should do it as long as that layer doesn't already exist (otherwise it'll come out asprimary1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a play - from memory I think a layer with the 'primary' name already exists (at least in my test samples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how I would/have done it. Definitely kinda of hacky, but I'm working on making the components of a config more reusable (by tagging their requirement type so it can be applied to "best guess" requirments of a similar type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if it's how you would have thought to do it that's got to be a compliment! :D I'll reword the TODO so it's worded more professionally and make a note to revisit it when you get time to add those config bits.